refactor: migrate from Result<T, String> to typed TaskError (#660)#739
Merged
Conversation
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>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates many modules from Changes
Sequence Diagram(s)sequenceDiagram
actor UI as UI
participant Task as BackendTaskExecutor
participant SDK as dash_sdk / Drive
participant DB as Database
UI->>Task: submit task
Task->>SDK: perform operation (state transition / broadcast)
alt Drive proof error
SDK-->>Task: DriveProofError
Task->>DB: persist ProofLogItem via log_drive_proof_error (rgba(200,100,50,0.5))
Task-->>UI: TaskResult::Error(TaskError::ProofError) (Display)
UI->>UI: show banner (Display) + expandable details (Debug)
else domain error
SDK-->>Task: domain error
Task-->>UI: TaskResult::Error(TaskError::from(domain))
else success
SDK-->>Task: Success
Task->>DB: persist result if needed
Task-->>UI: TaskResult::Success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
lklimek
changed the title
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>
…#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>
…e/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>
…#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>
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>
…ch (#660) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Migrates large parts of the codebase from Result<T, String> to a typed TaskError (and related domain errors) to preserve error sources and improve user-facing messaging consistency.
Changes:
- Added/extended structured error types (
TaskError,WalletError,DashPayError,ConfigError,P2PError) and updated conversions. - Replaced many
.map_err(|e| e.to_string())callsites with typed error propagation across UI, context, backend tasks, and components. - Updated UI flows to use structured error messages and safer state handling (e.g., deferred error dismissal).
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/tokens/tokens_screen/structs.rs | Returns TaskError from token lookup constructors and propagates typed errors. |
| src/ui/dashpay/contact_requests.rs | Avoids cloning/mutating self.error mid-UI render; uses user_message() once. |
| src/ui/dashpay/contact_profile_viewer.rs | Maps DB errors into TaskError::Database. |
| src/ui/dashpay/add_contact_screen.rs | Simplifies success state and removes unused derives. |
| src/model/wallet/utxos.rs | Introduces WalletError contexts for address parse failures (still stringified). |
| src/model/wallet/mod.rs | Adds WalletError key-derivation / sighash contexts (still stringified). |
| src/database/wallet.rs | Extends WalletError with KeyDerivation and Sighash variants. |
| src/context/wallet_lifecycle.rs | Improves lock-poison mapping and DB error mapping to TaskError (stringified here). |
| src/context/transaction_processing.rs | Uses TaskError for lock poisoning + core RPC + DB errors (stringified here). |
| src/context/mod.rs | Standardizes lock-poison errors via TaskError::LockPoisoned (stringified here). |
| src/context/contract_token_db.rs | Improves wallet lock poisoning errors and DB error mapping. |
| src/config.rs | Adds ConfigError::SaveError and improves atomic save error handling. |
| src/components/core_p2p_handler.rs | Adds P2PError and converts P2P handler internals to typed errors. |
| src/backend_task/wallet/*.rs | Updates wallet backend tasks to return TaskError and propagate typed SDK errors. |
| src/backend_task/update_data_contract.rs | Moves contract-update task to TaskError and avoids stringifying send errors. |
| src/backend_task/register_contract.rs | Moves contract-register task to TaskError and avoids stringifying send errors. |
| src/backend_task/mod.rs | Updates wallet task runner signature to return TaskError. |
| src/backend_task/error.rs | Adds Database, CoreRpc, LockPoisoned, P2P variants + conversions + tests. |
| src/backend_task/document.rs | Converts document tasks to TaskError propagation and typed “not found”. |
| src/backend_task/dashpay/errors.rs | Extends DashPayError with new variants; drops unused derives. |
| src/backend_task/contract.rs | Converts contract tasks to TaskError (some remaining string formatting). |
| src/backend_task/broadcast_state_transition.rs | Converts to TaskError and uses TaskError::from for SDK errors. |
| CLAUDE.md | Promotes i18n-ready strings guideline; adjusts error-message rules ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
@copilot backend_task layer shouldn't return Generic error variant. It should define TaskError variants that make business sense for regular users and are actionable. See CLAUDE.md for guidelines. |
Contributor
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/model/wallet/utxos.rs (1)
186-205:⚠️ Potential issue | 🟠 MajorBuild a validated UTXO snapshot before mutating wallet state.
self.utxosis pruned before eitherAddress::from_script()call runs. If one script fails to decode,reload_utxos()returns with a partially rebuilt in-memory set and without completing the DB sync, so the wallet can stay out of sync until the next successful refresh. Please resolve scripts into a temporary address map first, then apply the validated snapshot.Also applies to: 212-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/wallet/utxos.rs` around lines 186 - 205, reload_utxos() currently prunes self.utxos before validating scripts, so a single Address::from_script() failure can leave the in-memory wallet partially mutated; fix by first building a temporary validated snapshot (e.g., temp_utxos: HashMap<Address, HashMap<OutPoint, TxOut>>) from new_utxo_map by converting every tx_out.script_pubkey via Address::from_script and collecting results, returning early on any AddressError; only after the entire temp_utxos is successfully built, swap/assign it to self.utxos and proceed with DB sync and any further updates (apply the same pattern for the other block referenced at lines ~212-230) so state is only mutated after full validation.src/backend_task/document.rs (1)
170-188:⚠️ Potential issue | 🟠 MajorDon't embed raw proof diagnostics in the banner message.
These
TaskError::Generic(format!(... {proof_error} ...))branches now go straight throughDisplay, so protocol/debug text will be shown to end users even though the proof bytes are already logged locally. Return a fixed user-facing message here and keep the proof diagnostic in a typed variant/source chain or only in the stored proof log.As per coding guidelines:
src/backend_task/**/*.rs: backend tasks returnResult<T, TaskError>withDisplayproducing user-friendly text forMessageBannerandDebugcarrying the technical details.Also applies to: 225-244, 281-300, 355-374, 430-449, 504-523
src/context/wallet_lifecycle.rs (1)
29-44:⚠️ Potential issue | 🟠 MajorDon't report a successful clear if the in-memory state wasn't actually cleared.
After
clear_network_data()succeeds, both cache-clearing branches still swallow lock failures and the function returnsOk(())anyway. That can leave stale wallets in memory while the caller shows a success message.Suggested fix
- if let Ok(mut wallets) = self.wallets.write() { - wallets.clear(); - } - - if let Ok(mut single_key_wallets) = self.single_key_wallets.write() { - single_key_wallets.clear(); - } + self.wallets + .write() + .map_err(|_| TaskError::LockPoisoned { resource: "wallets" }.to_string())? + .clear(); + + self.single_key_wallets + .write() + .map_err(|_| TaskError::LockPoisoned { resource: "single_key_wallets" }.to_string())? + .clear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/wallet_lifecycle.rs` around lines 29 - 44, The function clear_network_database currently ignores failures to acquire the write locks for self.wallets and self.single_key_wallets and returns Ok even if in-memory state wasn't cleared; change the two `if let Ok(mut wallets) = self.wallets.write()` and `if let Ok(mut single_key_wallets) = self.single_key_wallets.write()` blocks to propagate an Err when the RwLock write fails (do not swallow the PoisonError). In practice replace those `if let` checks with calls that map the lock error into the function Result (e.g., using `self.wallets.write().map_err(|e| TaskError::Lock{ source: e }.to_string())?` and similarly for `self.single_key_wallets.write()`), then clear the guards and continue; this ensures `clear_network_database` only returns Ok(()) when both the DB and in-memory caches were actually cleared.src/backend_task/update_data_contract.rs (1)
150-165:⚠️ Potential issue | 🟠 MajorDon't report a recovered insert if
replace_contract()is ignored.In this recovery path,
replace_contract(...).ok()can fail silently, but the returnedTaskError::Generic(...)still says the contract was inserted. That can leave local state stale, mislead the user, and throw away the proof/database source information this refactor is trying to preserve. Please propagate the DB write as a typed error, and if the replace succeeds, return a recovered/success result instead ofErrso callers don't prompt a duplicate retry.Based on learnings, database errors use
TaskError, DET error refactors should propagate failures with?, and backend-task-originated errors should generally live in typedTaskErrorvariants rather thanGeneric.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/update_data_contract.rs` around lines 150 - 165, The recovery branch currently calls self.db.replace_contract(contract.id(), &contract, self).ok() which silently swallows DB errors while still returning a TaskError::Generic that claims the contract was inserted; change this so the replace_contract result is propagated as a typed TaskError (use the existing TaskError variants or add one for DB write failures) instead of using .ok(), using ? to propagate errors from replace_contract; if replace_contract returns Ok then return a recovered/success result (not Err) so callers know the state was fixed, and if it returns Err convert/wrap that error into the appropriate TaskError variant rather than still returning the misleading Generic error (update the code paths around DataContract::fetch, replace_contract, and the TaskError::Generic return accordingly).src/components/core_p2p_handler.rs (1)
212-235:⚠️ Potential issue | 🟠 MajorRestore the QRInfo socket timeout on every return path.
The temporary longer read timeout is reset on success and on the explicit timeout branch, but
Err(ReadMessageError::Fatal(e))returns early without restoring it. After a fatal read/protocol error, this handler keeps the enlarged timeout for later requests, which can make subsequent calls block much longer than intended.♻️ Proposed fix
let previous_socket_timeout = self.stream.read_timeout()?; self.stream.set_read_timeout(Some(socket_timeout))?; - let start_time = std::time::Instant::now(); - let timeout = overall_timeout; - loop { - if start_time.elapsed() > timeout { - self.stream.set_read_timeout(previous_socket_timeout)?; - return Err(P2PError::Timeout); - } - match self.read_message() { + let result = (|| -> Result<QRInfo, P2PError> { + let start_time = std::time::Instant::now(); + let timeout = overall_timeout; + loop { + if start_time.elapsed() > timeout { + return Err(P2PError::Timeout); + } + match self.read_message() { Ok((c, p)) => { command = c; payload = p; } Err(ReadMessageError::Transient) => { thread::sleep(Duration::from_millis(10)); continue; } Err(ReadMessageError::Fatal(e)) => return Err(e), } if command == "qrinfo" { tracing::debug!("Got qrinfo message"); - self.stream.set_read_timeout(previous_socket_timeout)?; break; } else { thread::sleep(Duration::from_millis(10)); } } let response_message: RawNetworkMessage = deserialize(&payload)?; match response_message.payload { NetworkMessage::QRInfo(qr_info) => Ok(qr_info), msg => Err(P2PError::UnexpectedMessage { received: format!("{msg:?}"), }), } - } + })(); + self.stream.set_read_timeout(previous_socket_timeout)?; + result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/core_p2p_handler.rs` around lines 212 - 235, The temporary read timeout set via self.stream.set_read_timeout(Some(socket_timeout)) is not restored on the ReadMessageError::Fatal(e) early return, leaving the stream with an enlarged timeout; update the logic in the loop that calls self.read_message() (handling ReadMessageError::Transient and ReadMessageError::Fatal) so that previous_socket_timeout is always restored before any return or break (e.g., use a RAII/scope guard or call self.stream.set_read_timeout(previous_socket_timeout)? before returning Err(e) in the ReadMessageError::Fatal branch and before any other early returns), ensuring the timeout is reset for subsequent calls (affecting the call sites: read_message, ReadMessageError::Fatal, P2PError::Timeout, and the qrinfo success path).
🧹 Nitpick comments (3)
src/backend_task/dashpay/errors.rs (1)
142-151: Consider adding new variants torequires_user_action().
PrivateKeyResolutionexplicitly instructs users to "Try refreshing your identities", andCryptoKeyParsingsuggests data corruption that may require user intervention. For consistency with the method's purpose, these could be added:fn requires_user_action(&self) -> bool { matches!( self, DashPayError::UsernameResolutionFailed { .. } | DashPayError::InvalidQrCode { .. } | DashPayError::QrCodeExpired { .. } | DashPayError::ValidationFailed { .. } | DashPayError::AccountLabelTooLong { .. } | DashPayError::InvalidUsername { .. } | DashPayError::MissingField { .. } | DashPayError::MissingEncryptionKey | DashPayError::MissingDecryptionKey + | DashPayError::PrivateKeyResolution { .. } + | DashPayError::CryptoKeyParsing { .. } ) }This is optional for this PR since it's focused on the migration infrastructure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/dashpay/errors.rs` around lines 142 - 151, Add the new error variants to the requires_user_action() match so PrivateKeyResolution and CryptoKeyParsing return true; specifically update the requires_user_action() function (the match over the error enum) to include arms for PrivateKeyResolution { .. } and CryptoKeyParsing { .. } that yield true, while keeping existing behavior for other variants unchanged so these two error types are treated as requiring user intervention.src/context/mod.rs (1)
328-333: Optional cleanup: deduplicate lock-poison string conversion.These mappings are repeated many times; a tiny helper would reduce noise and avoid drift.
♻️ Optional refactor sketch
impl AppContext { + #[inline] + fn lock_poisoned(resource: &'static str) -> String { + TaskError::LockPoisoned { resource }.to_string() + } + pub fn reinit_core_client_and_sdk(self: Arc<Self>) -> Result<(), String> { let cfg = { let cfg_lock = self .config .read() - .map_err(|_| TaskError::LockPoisoned { resource: "config" }.to_string())?; + .map_err(|_| Self::lock_poisoned("config"))?;Also applies to: 345-350, 406-411, 422-427, 518-523, 533-538, 547-552, 562-567, 572-577
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/mod.rs` around lines 328 - 333, Multiple call sites convert a poisoned lock into the same TaskError::LockPoisoned string (e.g., the map_err closures producing TaskError::LockPoisoned { resource: "spv_context_provider" }.to_string()); factor this into a small helper like lock_poisoned_err(resource: &str) -> String and replace the repeated map_err closures with map_err(|_| lock_poisoned_err("spv_context_provider")) (and same for the other resource names referenced in the comment) to deduplicate the conversion and reduce duplication across the file.src/components/core_p2p_handler.rs (1)
85-87: The newP2PErrorstill gets trapped behind string wrappers.
CoreP2PHandler::new(),send_dml_request_message(), andsend_qr_info_request_message()immediatelyto_string()the new error type. Callers going through the normal API—e.g.src/backend_task/mnlist.rs:37-57—still can't use theTaskError::P2Pbridge fromsrc/backend_task/error.rs:88-91. If end-to-end typed propagation is in scope here, exposeP2PErrorat the public boundary and keep string conversion only at the final compatibility layer.Also applies to: 123-126, 185-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/core_p2p_handler.rs` around lines 85 - 87, CoreP2PHandler::new, send_dml_request_message, and send_qr_info_request_message are converting the new P2PError into String immediately, which prevents callers (e.g. code in backend_task/mnlist.rs relying on TaskError::P2P) from seeing the typed error; change these public APIs to return Result<..., P2PError> (or otherwise propagate P2PError) instead of Result<..., String>, remove the .to_string() mapping in CoreP2PHandler::new, send_dml_request_message, and send_qr_info_request_message, and only convert to String at the outer compatibility layer that implements TaskError conversion, so callers can construct TaskError::P2P from the concrete P2PError.
🤖 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/contract.rs`:
- Line 68: The code is converting upstream errors into ad-hoc Strings (e.g. the
branch with Err(e) => Err(format!("Error fetching contracts: {}", e).into())),
which collapses typed errors; add specific TaskError variants (for example
TaskError::BackendFetchContracts(Box<dyn std::error::Error + Send + Sync>) and
TaskError::BackendRemoveContract(...)) to the TaskError enum and replace these
String/Generic conversions with returning those concrete variants (e.g. return
Err(TaskError::BackendFetchContracts(e.into())) or propagate the upstream error
type directly via map_err(Into::into)); apply the same fix for the other similar
branches mentioned (the remove/fetch failure sites) so callers can match on
structured TaskError values instead of Generic/String errors.
In `@src/backend_task/register_contract.rs`:
- Around line 101-111: Replace the user-facing Generic error messages that
interpolate proof_error in register_contract.rs (the two uses of
TaskError::Generic around the "Error broadcasting Register Contract transition"
branches) with a granular TaskError variant (e.g.,
TaskError::RegisterContractProof or TaskError::BroadcastError) that does not
expose raw proof_error in its Display text; log or attach the original
proof_error to internal/debug logging or to the variant's internal field (for
programmatic matching) but ensure the message returned to users is generic and
non-technical.
In `@src/backend_task/wallet/fetch_platform_address_balances.rs`:
- Around line 24-26: The code currently maps missing/locked/poisoned wallet
errors to TaskError::Generic via the
wallets.get(&seed_hash).cloned().ok_or_else(...) call; replace these generic
string cases with concrete error variants (e.g., TaskError::WalletNotFound or a
WalletError-based variant) and propagate lock-poison errors through the
dedicated poison branch you added (map PoisonError to TaskError::LockPoisoned or
TaskError::WalletLockPoison with #[source] pointing to the poison cause). Update
the ok_or_else and similar branches at the other locations you flagged (around
lines referenced 35-49 and 124-128) to return the specific TaskError/WalletError
variants instead of TaskError::Generic(String), preserving user-friendly Display
text and attaching the original source error where appropriate.
In `@src/backend_task/wallet/fund_platform_address_from_asset_lock.rs`:
- Around line 28-52: Replace the panicking .read().unwrap() on the wallets
RwLock with .read().map_err(...) and return TaskError::LockPoisoned { resource:
"wallets" } (instead of TaskError::Generic) for the wallets guard; similarly
replace the wallet_arc.read().map_err(...) error construction to return
TaskError::LockPoisoned { resource: "wallet_arc" } (not Generic) in the block
around wallet_arc and wallet cloning; change the "Wallet not found" and "Asset
lock address not found in wallet" Generic(...) errors to the more specific typed
TaskError variants used in the codebase (e.g., TaskError::NotFound { resource:
"wallet" } and TaskError::NotFound { resource: "asset_lock_key" } or the
appropriate existing variant) so missing-wallet and missing-key conditions are
typed; keep the existing .private_key_for_address(...).map_err(TaskError::from)?
propagation as-is.
In `@src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs`:
- Around line 42-53: Replace all uses of .read().unwrap() and manual
PoisonError-to-Generic mappings in this file with the crate's PoisonError
conversion: call self.wallets.read().map_err(|e| e.into())? (or simply use ?
where appropriate) instead of .unwrap(), and replace .map_err(|_|
TaskError::Generic(...)) on RwLock write/read locks (e.g., the
wallet_arc.write() call and other occurrences) with .map_err(|e| e.into())? so
the existing From<std::sync::PoisonError<T>> -> TaskError::LockPoisoned is used;
also replace ok_or_else(|| TaskError::Generic("Wallet not found"))? with a more
specific error (add/emit a WalletNotFound TaskError variant or use an existing
variant) for the wallets.get(&seed_hash).cloned() checks to avoid Generic error
usage.
In `@src/backend_task/wallet/generate_receive_address.rs`:
- Around line 12-16: Replace the panic-prone unwrap() and Generic error uses in
generate_receive_address.rs by mapping both RwLock read() failures to the typed
TaskError::LockPoisoned { resource: "..."} and replacing the Generic("Wallet not
found") with a dedicated not-found variant (e.g. TaskError::WalletNotFound {
seed_hash }) so callers can pattern-match; update the block that obtains wallets
(the wallets.read().unwrap() path and its subsequent
get(...).cloned().ok_or_else(...)) to return LockPoisoned on lock poisoning and
WalletNotFound on missing keys, and apply the same replacement for the other
lock acquisition at lines ~35-39 to ensure all lock and lookup failures use the
specific TaskError variants and preserve sources.
In `@src/backend_task/wallet/transfer_platform_credits.rs`:
- Around line 24-37: Replace the panic-prone .unwrap() on self.wallets.read()
with a safe map_err that returns a granular TaskError (e.g.,
TaskError::LockPoisoned or TaskError::ReadLockError) and replace the generic
"Wallet not found" Generic variant with a specific TaskError::WalletNotFound
(include seed_hash in the message/variant payload) when wallets.get(&seed_hash)
is None; keep the wallet_arc.read() mapping but ensure it maps to a specific
TaskError::InternalLockError rather than a generic string. For
update_wallet_platform_address_info_from_sdk(), either change its signature in
src/context/wallet_lifecycle.rs to return Result<(), TaskError> or at the call
site map its Err(String) into a granular TaskError (e.g., map_err(|s|
TaskError::WalletLifecycleError(s))) instead of relying on From<String>; update
all call sites to use the specific TaskError variants so backend_task functions
return Result<T, TaskError> with granular errors rather than panicking or using
TaskError::Generic.
In `@src/context/contract_token_db.rs`:
- Around line 160-169: Acquire the wallets write lock (self.wallets.write())
before deleting the DB row so in-memory and persistent state are mutated
atomically: lock and remove the wallet entry from the in-memory map (the
structure held in self.wallets keyed by seed_hash and self.network), then call
self.db.remove_wallet(seed_hash, &self.network); if the DB call fails, roll back
by reinserting the removed wallet into the in-memory map and return the
TaskError::Database as before; ensure you still map LockPoisoned to the same
TaskError and recompute/update has_wallet consistently after successful removal.
In `@src/ui/tokens/tokens_screen/structs.rs`:
- Around line 161-164: Replace terse TaskError::Generic messages produced in the
.ok_or_else(...) calls with clear, actionable, user-facing sentences;
specifically update the identity lookup error (the .ok_or_else(||
TaskError::Generic("Identity not found".to_string()))? call) to something like
"Unable to find the requested identity — please confirm your account selection
or refresh and try again" and update the contract lookup error (the
get_contract_by_id(...)? .ok_or_else(|| TaskError::Generic("Contract not
found".to_string()))? call) to something like "Requested contract could not be
found — verify the contract ID and try reloading the data"; keep using
TaskError::Generic but replace the string content, and prefer including the
queried ID in the message for debugging if available.
---
Outside diff comments:
In `@src/backend_task/update_data_contract.rs`:
- Around line 150-165: The recovery branch currently calls
self.db.replace_contract(contract.id(), &contract, self).ok() which silently
swallows DB errors while still returning a TaskError::Generic that claims the
contract was inserted; change this so the replace_contract result is propagated
as a typed TaskError (use the existing TaskError variants or add one for DB
write failures) instead of using .ok(), using ? to propagate errors from
replace_contract; if replace_contract returns Ok then return a recovered/success
result (not Err) so callers know the state was fixed, and if it returns Err
convert/wrap that error into the appropriate TaskError variant rather than still
returning the misleading Generic error (update the code paths around
DataContract::fetch, replace_contract, and the TaskError::Generic return
accordingly).
In `@src/components/core_p2p_handler.rs`:
- Around line 212-235: The temporary read timeout set via
self.stream.set_read_timeout(Some(socket_timeout)) is not restored on the
ReadMessageError::Fatal(e) early return, leaving the stream with an enlarged
timeout; update the logic in the loop that calls self.read_message() (handling
ReadMessageError::Transient and ReadMessageError::Fatal) so that
previous_socket_timeout is always restored before any return or break (e.g., use
a RAII/scope guard or call
self.stream.set_read_timeout(previous_socket_timeout)? before returning Err(e)
in the ReadMessageError::Fatal branch and before any other early returns),
ensuring the timeout is reset for subsequent calls (affecting the call sites:
read_message, ReadMessageError::Fatal, P2PError::Timeout, and the qrinfo success
path).
In `@src/context/wallet_lifecycle.rs`:
- Around line 29-44: The function clear_network_database currently ignores
failures to acquire the write locks for self.wallets and self.single_key_wallets
and returns Ok even if in-memory state wasn't cleared; change the two `if let
Ok(mut wallets) = self.wallets.write()` and `if let Ok(mut single_key_wallets) =
self.single_key_wallets.write()` blocks to propagate an Err when the RwLock
write fails (do not swallow the PoisonError). In practice replace those `if let`
checks with calls that map the lock error into the function Result (e.g., using
`self.wallets.write().map_err(|e| TaskError::Lock{ source: e }.to_string())?`
and similarly for `self.single_key_wallets.write()`), then clear the guards and
continue; this ensures `clear_network_database` only returns Ok(()) when both
the DB and in-memory caches were actually cleared.
In `@src/model/wallet/utxos.rs`:
- Around line 186-205: reload_utxos() currently prunes self.utxos before
validating scripts, so a single Address::from_script() failure can leave the
in-memory wallet partially mutated; fix by first building a temporary validated
snapshot (e.g., temp_utxos: HashMap<Address, HashMap<OutPoint, TxOut>>) from
new_utxo_map by converting every tx_out.script_pubkey via Address::from_script
and collecting results, returning early on any AddressError; only after the
entire temp_utxos is successfully built, swap/assign it to self.utxos and
proceed with DB sync and any further updates (apply the same pattern for the
other block referenced at lines ~212-230) so state is only mutated after full
validation.
---
Nitpick comments:
In `@src/backend_task/dashpay/errors.rs`:
- Around line 142-151: Add the new error variants to the requires_user_action()
match so PrivateKeyResolution and CryptoKeyParsing return true; specifically
update the requires_user_action() function (the match over the error enum) to
include arms for PrivateKeyResolution { .. } and CryptoKeyParsing { .. } that
yield true, while keeping existing behavior for other variants unchanged so
these two error types are treated as requiring user intervention.
In `@src/components/core_p2p_handler.rs`:
- Around line 85-87: CoreP2PHandler::new, send_dml_request_message, and
send_qr_info_request_message are converting the new P2PError into String
immediately, which prevents callers (e.g. code in backend_task/mnlist.rs relying
on TaskError::P2P) from seeing the typed error; change these public APIs to
return Result<..., P2PError> (or otherwise propagate P2PError) instead of
Result<..., String>, remove the .to_string() mapping in CoreP2PHandler::new,
send_dml_request_message, and send_qr_info_request_message, and only convert to
String at the outer compatibility layer that implements TaskError conversion, so
callers can construct TaskError::P2P from the concrete P2PError.
In `@src/context/mod.rs`:
- Around line 328-333: Multiple call sites convert a poisoned lock into the same
TaskError::LockPoisoned string (e.g., the map_err closures producing
TaskError::LockPoisoned { resource: "spv_context_provider" }.to_string());
factor this into a small helper like lock_poisoned_err(resource: &str) -> String
and replace the repeated map_err closures with map_err(|_|
lock_poisoned_err("spv_context_provider")) (and same for the other resource
names referenced in the comment) to deduplicate the conversion and reduce
duplication across the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d2a7edb-c35b-4225-b09f-792fc3b47a69
📒 Files selected for processing (28)
CLAUDE.mdsrc/backend_task/broadcast_state_transition.rssrc/backend_task/contract.rssrc/backend_task/dashpay/errors.rssrc/backend_task/document.rssrc/backend_task/error.rssrc/backend_task/mod.rssrc/backend_task/register_contract.rssrc/backend_task/update_data_contract.rssrc/backend_task/wallet/fetch_platform_address_balances.rssrc/backend_task/wallet/fund_platform_address_from_asset_lock.rssrc/backend_task/wallet/fund_platform_address_from_wallet_utxos.rssrc/backend_task/wallet/generate_receive_address.rssrc/backend_task/wallet/transfer_platform_credits.rssrc/backend_task/wallet/withdraw_from_platform_address.rssrc/components/core_p2p_handler.rssrc/config.rssrc/context/contract_token_db.rssrc/context/mod.rssrc/context/transaction_processing.rssrc/context/wallet_lifecycle.rssrc/database/wallet.rssrc/model/wallet/mod.rssrc/model/wallet/utxos.rssrc/ui/dashpay/add_contact_screen.rssrc/ui/dashpay/contact_profile_viewer.rssrc/ui/dashpay/contact_requests.rssrc/ui/tokens/tokens_screen/structs.rs
…ck, 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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/backend_task/wallet/withdraw_from_platform_address.rs (1)
53-68:⚠️ Potential issue | 🔴 CriticalDon't report the withdrawal as failed after the side effect has already succeeded.
withdraw_address_funds()is the committed, non-idempotent step. If the follow-up balance refresh fails, this function returnsErr, which makes the caller think the withdrawal failed and can trigger a duplicate retry. Keep the task successful once the withdrawal is accepted, and surface refresh failure separately.💡 Suggested fix
let _result = sdk .withdraw_address_funds( inputs, None, // No change output fee_strategy, core_fee_per_byte, Pooling::Never, output_script, &wallet, None, ) .await .map_err(crate::backend_task::error::TaskError::from)?; // Trigger a balance refresh - self.fetch_platform_address_balances(seed_hash).await?; + if let Err(e) = self.fetch_platform_address_balances(seed_hash).await { + tracing::warn!( + "Platform withdrawal succeeded, but balance refresh failed for {:?}: {:?}", + &seed_hash, + e + ); + } Ok(BackendTaskSuccessResult::PlatformAddressWithdrawal { seed_hash })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/wallet/withdraw_from_platform_address.rs` around lines 53 - 68, The withdraw_address_funds() call is the committed, non-idempotent operation and its success must not be hidden by a subsequent failure in fetch_platform_address_balances(); change the flow in withdraw_from_platform_address so you first await and verify sdk.withdraw_address_funds(...) succeeded, then call self.fetch_platform_address_balances(seed_hash).await but do NOT convert a refresh error into the function's Err return—instead log or record the refresh failure (e.g., with error/warn), and return success (Ok) for the overall task when the withdrawal was accepted; ensure the code references the exact calls withdraw_address_funds and fetch_platform_address_balances when implementing this behavior.src/backend_task/register_contract.rs (1)
89-116:⚠️ Potential issue | 🟠 MajorReturn one durable result from the proof-recovery branch.
This path drops
insert_contract_if_not_existsfailures with.ok(), sendsContractSavedAfterProofError, and then still returnsErr(TaskError::ProofError { .. })torun_contract_task. That can leave the recovered contract unsaved or missing the user-supplied alias locally while the same action reports both success and failure. Persist with?, reusealias, and returnOk(BackendTaskSuccessResult::ContractSavedAfterProofError)once recovery succeeds.
As per coding guidelines:src/backend_task/**/*.rs:BackendTaskvariants must have correspondingrun_*_task()methods inAppContextthat returnResult<T, TaskError>, and results must be typed variants ofBackendTaskSuccessResult.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/register_contract.rs` around lines 89 - 116, The proof-recovery branch currently swallows insert_contract_if_not_exists errors, sends ContractSavedAfterProofError, then returns Err(TaskError::ProofError), causing inconsistent success/failure reporting; change the branch handling in run_contract_task so that after DataContract::fetch succeeds you retrieve the alias via get_contract_by_id, call insert_contract_if_not_exists and propagate errors (use ?), then send sender.send(TaskResult::Success(Box::new(BackendTaskSuccessResult::ContractSavedAfterProofError))).await? and finally return Ok(BackendTaskSuccessResult::ContractSavedAfterProofError) instead of Err(TaskError::ProofError) so the recovered contract (and alias) is persisted and a single durable success result is returned to the AppContext caller.src/backend_task/wallet/fund_platform_address_from_asset_lock.rs (1)
125-138:⚠️ Potential issue | 🟠 MajorDon't silently skip local asset-lock cleanup.
self.wallets.read().ok().and_then(...)turns both a poisoned wallets map and a missing wallet entry into a no-op after the top-up has already succeeded. In that caseunused_asset_locksnever gets pruned, so the current session can keep showing a spent asset lock as reusable until the wallet is reloaded. Please make this failure explicit instead of returning success with stale local state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/wallet/fund_platform_address_from_asset_lock.rs` around lines 125 - 138, The current use of self.wallets.read().ok().and_then(...) hides both a poisoned RwLock read and a missing wallet entry, so change it to explicitly handle errors and missing entries: call self.wallets.read().map_err(|_| TaskError::LockPoisoned { resource: "wallets" })? to fail on a poisoned map, then if the read succeeds try to get the wallet with .get(&seed_hash).cloned() and return a TaskError::WalletNotFound (or another appropriate explicit TaskError including seed_hash) if it's absent; once you have wallet_arc, keep the existing wallet_arc.write().map_err(|_| TaskError::LockPoisoned { resource: "wallet" })? and the unused_asset_locks.retain(...) logic unchanged so local cleanup is performed or an explicit error is returned.src/backend_task/update_data_contract.rs (1)
120-141:⚠️ Potential issue | 🟡 MinorDon't emit
ProofErrorLoggedafter discarding the insert result.
insert_proof_log_item(...).ok()makes the write best-effort, but the next block always sendsBackendTaskSuccessResult::ProofErrorLogged. If that insert fails, the UI reports a saved proof log that doesn't exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/update_data_contract.rs` around lines 120 - 141, The code currently discards the result of self.db.insert_proof_log_item(...) using .ok() but always sends BackendTaskSuccessResult::ProofErrorLogged; change this so you check the insert_proof_log_item result and only send ProofErrorLogged on Ok, and propagate/map the Err into the task error path (e.g., return or send a TaskError) on failure. Locate insert_proof_log_item and the sender.send(TaskResult::Success(Box::new(BackendTaskSuccessResult::ProofErrorLogged))) call and replace the unconditional .ok() + success send with a match/if let that handles Err by mapping it to an appropriate crate::backend_task::error::TaskError and only sends ProofErrorLogged when the DB insert succeeded. Ensure the reconstructed source_error handling remains unchanged.src/backend_task/identity/register_identity.rs (1)
607-650: 🛠️ Refactor suggestion | 🟠 MajorReuse the centralized proof-error mapping for address-funded registration.
This branch reimplements proof logging and then returns formatted strings for both proof and non-proof failures. That bypasses
TaskError::ProofError, loses the source chain, and can still claim the proof was logged after a failed insert.self.log_drive_proof_error(e)plus typed propagation for the fallback branch would keep this path consistent with the rest of the migration.As per coding guidelines,
src/backend_task/**/*.rs: "Prefer granularTaskErrorvariants with#[source]fields overTaskError::Generic(format!(...))to preserve error chains, enable structural matching, and makeDisplay/Debugseparation explicit."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/register_identity.rs` around lines 607 - 650, Replace the manual DriveProofError handling and string-based TaskError construction with the centralized proof-error helper and typed TaskError propagation: call self.log_drive_proof_error(e) when the error matches Error::DriveProofError (and let that helper persist the ProofLogItem), keep the qualified_identity.status update and the self.insert_local_qualified_identity(...) call, then return or propagate a TaskError::ProofError (or the concrete TaskError variant your log helper returns) using the error value (not format! string) so the source chain is preserved; for the fallback non-proof branch call self.log_drive_proof_error(e) or map the original error into the appropriate TaskError variant (using ?/From) instead of TaskError::from(format!(...)), so both branches use the same typed TaskError and keep the original source.src/context/wallet_lifecycle.rs (1)
29-38:⚠️ Potential issue | 🟠 MajorDon’t return success if wallet maps couldn’t be cleared
clear_network_database()now returnsTaskError, but Line 32 and Line 36 still swallow poisoned lock errors and may leave in-memory state uncleared while returningOk(()).Suggested fix
- if let Ok(mut wallets) = self.wallets.write() { - wallets.clear(); - } + let mut wallets = self.wallets.write()?; + wallets.clear(); - if let Ok(mut single_key_wallets) = self.single_key_wallets.write() { - single_key_wallets.clear(); - } + let mut single_key_wallets = self.single_key_wallets.write()?; + single_key_wallets.clear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/wallet_lifecycle.rs` around lines 29 - 38, The clear_network_database function currently swallows poisoned lock errors when acquiring self.wallets.write() and self.single_key_wallets.write(), returning Ok(()) even if in-memory maps weren't cleared; change those calls to propagate an error instead of ignoring the Err variant (e.g., convert the PoisonError into a TaskError and return Err), so that failures to acquire the write lock for wallets or single_key_wallets cause clear_network_database to return Err(TaskError) rather than silently succeeding; update the handling around self.wallets.write() and self.single_key_wallets.write() in clear_network_database to map or ? the Err case into a TaskError with a clear message and only clear the maps after successful lock acquisition.src/backend_task/identity/load_identity.rs (1)
68-73: 🛠️ Refactor suggestion | 🟠 MajorAvoid stringifying errors in backend_task flows
These paths still convert failures into string-backed
TaskError(format!/.to_string()), which drops structured matching and weakens source-chain diagnostics. Introduce/route through concreteTaskErrorvariants with source types instead.As per coding guidelines
src/backend_task/**/*.rs: “Prefer granularTaskErrorvariants with#[source]fields overTaskError::Generic(format!(...))to preserve error chains…”.Also applies to: 181-183, 204-205, 319-319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/load_identity.rs` around lines 68 - 73, The code converts Identifier parsing failures into a string-backed TaskError which loses the original error chain; instead create or use a concrete TaskError variant (e.g., IdentifierParse or InvalidIdentifier) that carries the original parse error as a #[source] field and return that variant instead of TaskError::from(format!(...)); update the matching around Identifier::from_string(...) to map Err(e) => return Err(TaskError::IdentifierParse { source: e }) (or the existing appropriate variant) and make equivalent changes for the other occurrences noted (the Identifier parsing sites at the other locations).
🧹 Nitpick comments (3)
src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs (1)
174-178: Preserve the address-conversion source here.
PlatformAddress::try_from(addr)already yields a concrete error, butAddressConversionFailed { detail: e.to_string() }flattens it back into text. Carrying the source error inTaskErrorwould preserve matching/debugging while keepingDisplayuser-friendly.
As per coding guidelines:src/backend_task/**/*.rs: Prefer granularTaskErrorvariants with#[source]fields overTaskError::Generic(format!(...))to preserve error chains, enable structural matching, and makeDisplay/Debugseparation explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs` around lines 174 - 178, Change the AddressConversionFailed variant to carry the original conversion error as a source instead of only a detail string and pass the concrete error from PlatformAddress::try_from into that source field; update TaskError::AddressConversionFailed to include a #[source] field (e.g. source: Box<dyn std::error::Error + Send + Sync>) while keeping an optional detail/display string, and in the map_err call that wraps PlatformAddress::try_from(addr) construct AddressConversionFailed by boxing the original error (Box::new(e) or e.into()) so the error chain is preserved for matching/debugging while Display remains user-friendly.src/backend_task/tokens/update_token_config.rs (1)
89-93: Use a contract-specific not-found error in this fetch pathLine 92 maps a missing fetched data contract to
TaskError::DocumentNotFound, which weakens structural matching and user messaging for this backend task. Prefer a contract-specific variant (ideally carrying contract id context).As per coding guidelines
src/backend_task/**/*.rs: “Prefer granularTaskErrorvariants … to preserve error chains [and] enable structural matching.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/tokens/update_token_config.rs` around lines 89 - 93, The fetch of the data contract via DataContract::fetch(...).await currently maps a missing contract to the generic TaskError::DocumentNotFound; change this to a contract-specific error (e.g., TaskError::ContractNotFound(contract_id) or similar) so callers can structurally match and retain context — update the ok_or/map_err on the DataContract::fetch result in update_token_config.rs to produce the new contract-specific variant (and add the new TaskError variant carrying the contract id in the TaskError enum and adjust any From/Display impls accordingly).src/context/wallet_lifecycle.rs (1)
577-577: Preserve SPV wallet-manager error sources instead of stringifyingThese conversions (
SpvError::WalletError(e.to_string())) drop source structure. Prefer a typedSpvErrorvariant with#[source](or directFromconversion) soDebugretains full diagnostics.Also applies to: 626-626, 748-748
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/wallet_lifecycle.rs` at line 577, The code currently converts underlying wallet-manager errors to SpvError::WalletError by stringifying (e.to_string()), losing the original source; change the SpvError::WalletError variant to carry the concrete error (or a boxed dyn Error) with a #[source] attribute (e.g. WalletError(#[source] WalletManagerError) or WalletError(#[source] Box<dyn std::error::Error + Send + Sync>)), add a From<WalletManagerError> -> SpvError impl if needed, and update the map_err calls (the ones creating SpvError::WalletError currently using e.to_string()) to forward the original error (e or e.into()) so the source chain is preserved (update all occurrences including the map_err sites that produce SpvError::WalletError).
🤖 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/identity/load_identity_by_dpns_name.rs`:
- Around line 162-165: The code is silently dropping wallets when the RwLock is
poisoned by calling wallet.read().ok() and re-reading to get seed_hash(); since
the map is already keyed by WalletSeedHash, stop re-opening the lock and instead
build associated_wallets from the map entries directly—replace the filter_map
using wallet.read().ok().map(|w| (w.seed_hash(), wallet.clone())) with an
iterator over wallets that uses the existing key:
wallets.iter().map(|(seed_hash, wallet)| (seed_hash.clone(),
wallet.clone())).collect(), avoiding poisoned-lock reads and redundant seed_hash
lookups.
In `@src/backend_task/identity/load_identity.rs`:
- Around line 341-342: The code currently swallows poisoned RwLock reads
(wallet.read().ok() and the continue path in the key-matching loop), which can
hide failures; replace those .ok()/continue usages with explicit handling that
maps PoisonError into TaskError::LockPoisoned and returns it instead of silently
skipping. Concretely, update the closure used where you build the collection
from .filter_map(|wallet| wallet.read().ok().map(|w| (w.seed_hash(),
wallet.clone()))) to attempt wallet.read().map(|w| (w.seed_hash(),
wallet.clone())).map_err(|_| TaskError::LockPoisoned)? (or use a match to
convert PoisonError to TaskError::LockPoisoned), and likewise in the
key-matching loop that currently does continue on .read().ok() convert that
.ok() path into returning Err(TaskError::LockPoisoned) so any poisoned lock
surfaces up instead of being ignored; reference the seed_hash() call,
wallet.clone(), and the key-matching logic in load_identity.rs when making the
change.
In `@src/backend_task/identity/register_identity.rs`:
- Around line 52-58: Replace the two raw-string TaskError constructions in
register_identity.rs with concrete TaskError variants so callers can match on
them: where the code currently does TaskError::from("Asset Lock not valid for
wallet".to_string()) (near the private_key_for_address(&address, self.network)?
call) return a dedicated variant such as TaskError::AssetLockAddressNotFound or
TaskError::UserInput with an appropriate #[source] payload instead of a raw
string; do the same for the other occurrences referenced (around lines 408-411)
so wallet.read(), wallet.seed_hash(), and private_key_for_address(...) error
paths produce typed TaskError variants preserving the original error as the
source for chaining.
- Around line 145-152: The code is stringifying SDK/protocol errors when mapping
AddressInfo::fetch_many(...) and similar calls, which drops the source/error
chain; update these mappings to propagate the original error (e.g., use
TaskError::from(e) or TaskError::Sdk { #[source] source: e }), and for
constructor/protocol failures create and use a granular TaskError variant with a
#[source] field instead of format!(...) so the source chain is preserved; apply
this change for the AddressInfo::fetch_many branch shown and the other
occurrences mentioned (around lines referenced: 254-255 and 545-550) so callers
can structurally match and retain the original error types.
In `@src/backend_task/identity/top_up_identity.rs`:
- Around line 46-51: The current asset-lock failure paths convert errors to
TaskError::Generic via TaskError::from(String); instead, adjust the failure
returns in the private_key_for_address handling (the block using wallet.read()
and wallet.private_key_for_address(&address, self.network)) to return the
specific variants: return TaskError::AssetLockAddressNotFound when
private_key_for_address yields None (missing key) and return
TaskError::UserInput (or the new dedicated asset-lock TaskError variant) for the
expired-but-not-chainlocked case; make the same changes for the other occurrence
around the 266-269 region so both branches preserve structured TaskError
variants (with #[source] where appropriate) rather than converting to Generic.
In `@src/backend_task/tokens/mod.rs`:
- Around line 558-562: Replace the use of TaskError::Generic when
bincode::encode_to_vec fails for token_config_bytes: introduce or use a specific
TaskError variant (e.g., TaskError::TokenSerialization or
TaskError::BincodeEncode) that has a #[source] field to preserve the original
bincode error, and map the failure like .map_err(|e|
TaskError::TokenSerialization { source: e.into() }) when encoding
token_info.token_configuration; this keeps the error chain, enables structural
matching, and avoids exposing raw string errors.
In `@src/context/mod.rs`:
- Around line 709-718: The DB insert in log_drive_proof_error is being
fire-and-forget (let _ = self.db.insert_proof_log_item(...)), so insert failures
are hidden while callers still get TaskError::ProofError; change this to check
the Result from insert_proof_log_item and handle failures: if
insert_proof_log_item returns Err, emit a warning/error via the existing logger
(or other error reporting) that includes the insert error and context (e.g.,
ProofLogItem metadata), and fall back to a non-logging response path if desired
so callers are not misled; ensure you update log_drive_proof_error to propagate
the original TaskError::ProofError while also logging any DB insert Err,
referencing insert_proof_log_item, ProofLogItem, log_drive_proof_error, and
TaskError::ProofError.
In `@src/context/transaction_processing.rs`:
- Around line 57-63: The poisoned mutex error from
transactions_waiting_for_finality is being swallowed by the conditional and
treated like a timeout; change the logic to propagate the mapped
TaskError::LockPoisoned instead of skipping on Err — e.g. use the Result from
self.transactions_waiting_for_finality.lock().map_err(|_|
TaskError::LockPoisoned{ resource: "transactions_waiting_for_finality" }) and
propagate it (using ? or an explicit match that returns Err) so that a poisoned
lock returns Err(TaskError::LockPoisoned) immediately rather than falling
through and eventually producing TaskError::ConfirmationTimeout when checking
for tx_id/proof.
---
Outside diff comments:
In `@src/backend_task/identity/load_identity.rs`:
- Around line 68-73: The code converts Identifier parsing failures into a
string-backed TaskError which loses the original error chain; instead create or
use a concrete TaskError variant (e.g., IdentifierParse or InvalidIdentifier)
that carries the original parse error as a #[source] field and return that
variant instead of TaskError::from(format!(...)); update the matching around
Identifier::from_string(...) to map Err(e) => return
Err(TaskError::IdentifierParse { source: e }) (or the existing appropriate
variant) and make equivalent changes for the other occurrences noted (the
Identifier parsing sites at the other locations).
In `@src/backend_task/identity/register_identity.rs`:
- Around line 607-650: Replace the manual DriveProofError handling and
string-based TaskError construction with the centralized proof-error helper and
typed TaskError propagation: call self.log_drive_proof_error(e) when the error
matches Error::DriveProofError (and let that helper persist the ProofLogItem),
keep the qualified_identity.status update and the
self.insert_local_qualified_identity(...) call, then return or propagate a
TaskError::ProofError (or the concrete TaskError variant your log helper
returns) using the error value (not format! string) so the source chain is
preserved; for the fallback non-proof branch call self.log_drive_proof_error(e)
or map the original error into the appropriate TaskError variant (using ?/From)
instead of TaskError::from(format!(...)), so both branches use the same typed
TaskError and keep the original source.
In `@src/backend_task/register_contract.rs`:
- Around line 89-116: The proof-recovery branch currently swallows
insert_contract_if_not_exists errors, sends ContractSavedAfterProofError, then
returns Err(TaskError::ProofError), causing inconsistent success/failure
reporting; change the branch handling in run_contract_task so that after
DataContract::fetch succeeds you retrieve the alias via get_contract_by_id, call
insert_contract_if_not_exists and propagate errors (use ?), then send
sender.send(TaskResult::Success(Box::new(BackendTaskSuccessResult::ContractSavedAfterProofError))).await?
and finally return Ok(BackendTaskSuccessResult::ContractSavedAfterProofError)
instead of Err(TaskError::ProofError) so the recovered contract (and alias) is
persisted and a single durable success result is returned to the AppContext
caller.
In `@src/backend_task/update_data_contract.rs`:
- Around line 120-141: The code currently discards the result of
self.db.insert_proof_log_item(...) using .ok() but always sends
BackendTaskSuccessResult::ProofErrorLogged; change this so you check the
insert_proof_log_item result and only send ProofErrorLogged on Ok, and
propagate/map the Err into the task error path (e.g., return or send a
TaskError) on failure. Locate insert_proof_log_item and the
sender.send(TaskResult::Success(Box::new(BackendTaskSuccessResult::ProofErrorLogged)))
call and replace the unconditional .ok() + success send with a match/if let that
handles Err by mapping it to an appropriate
crate::backend_task::error::TaskError and only sends ProofErrorLogged when the
DB insert succeeded. Ensure the reconstructed source_error handling remains
unchanged.
In `@src/backend_task/wallet/fund_platform_address_from_asset_lock.rs`:
- Around line 125-138: The current use of self.wallets.read().ok().and_then(...)
hides both a poisoned RwLock read and a missing wallet entry, so change it to
explicitly handle errors and missing entries: call
self.wallets.read().map_err(|_| TaskError::LockPoisoned { resource: "wallets"
})? to fail on a poisoned map, then if the read succeeds try to get the wallet
with .get(&seed_hash).cloned() and return a TaskError::WalletNotFound (or
another appropriate explicit TaskError including seed_hash) if it's absent; once
you have wallet_arc, keep the existing wallet_arc.write().map_err(|_|
TaskError::LockPoisoned { resource: "wallet" })? and the
unused_asset_locks.retain(...) logic unchanged so local cleanup is performed or
an explicit error is returned.
In `@src/backend_task/wallet/withdraw_from_platform_address.rs`:
- Around line 53-68: The withdraw_address_funds() call is the committed,
non-idempotent operation and its success must not be hidden by a subsequent
failure in fetch_platform_address_balances(); change the flow in
withdraw_from_platform_address so you first await and verify
sdk.withdraw_address_funds(...) succeeded, then call
self.fetch_platform_address_balances(seed_hash).await but do NOT convert a
refresh error into the function's Err return—instead log or record the refresh
failure (e.g., with error/warn), and return success (Ok) for the overall task
when the withdrawal was accepted; ensure the code references the exact calls
withdraw_address_funds and fetch_platform_address_balances when implementing
this behavior.
In `@src/context/wallet_lifecycle.rs`:
- Around line 29-38: The clear_network_database function currently swallows
poisoned lock errors when acquiring self.wallets.write() and
self.single_key_wallets.write(), returning Ok(()) even if in-memory maps weren't
cleared; change those calls to propagate an error instead of ignoring the Err
variant (e.g., convert the PoisonError into a TaskError and return Err), so that
failures to acquire the write lock for wallets or single_key_wallets cause
clear_network_database to return Err(TaskError) rather than silently succeeding;
update the handling around self.wallets.write() and
self.single_key_wallets.write() in clear_network_database to map or ? the Err
case into a TaskError with a clear message and only clear the maps after
successful lock acquisition.
---
Nitpick comments:
In `@src/backend_task/tokens/update_token_config.rs`:
- Around line 89-93: The fetch of the data contract via
DataContract::fetch(...).await currently maps a missing contract to the generic
TaskError::DocumentNotFound; change this to a contract-specific error (e.g.,
TaskError::ContractNotFound(contract_id) or similar) so callers can structurally
match and retain context — update the ok_or/map_err on the DataContract::fetch
result in update_token_config.rs to produce the new contract-specific variant
(and add the new TaskError variant carrying the contract id in the TaskError
enum and adjust any From/Display impls accordingly).
In `@src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs`:
- Around line 174-178: Change the AddressConversionFailed variant to carry the
original conversion error as a source instead of only a detail string and pass
the concrete error from PlatformAddress::try_from into that source field; update
TaskError::AddressConversionFailed to include a #[source] field (e.g. source:
Box<dyn std::error::Error + Send + Sync>) while keeping an optional
detail/display string, and in the map_err call that wraps
PlatformAddress::try_from(addr) construct AddressConversionFailed by boxing the
original error (Box::new(e) or e.into()) so the error chain is preserved for
matching/debugging while Display remains user-friendly.
In `@src/context/wallet_lifecycle.rs`:
- Line 577: The code currently converts underlying wallet-manager errors to
SpvError::WalletError by stringifying (e.to_string()), losing the original
source; change the SpvError::WalletError variant to carry the concrete error (or
a boxed dyn Error) with a #[source] attribute (e.g. WalletError(#[source]
WalletManagerError) or WalletError(#[source] Box<dyn std::error::Error + Send +
Sync>)), add a From<WalletManagerError> -> SpvError impl if needed, and update
the map_err calls (the ones creating SpvError::WalletError currently using
e.to_string()) to forward the original error (e or e.into()) so the source chain
is preserved (update all occurrences including the map_err sites that produce
SpvError::WalletError).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b2d35568-384d-4724-a3bf-fa3973df1323
📒 Files selected for processing (37)
src/backend_task/contract.rssrc/backend_task/document.rssrc/backend_task/error.rssrc/backend_task/identity/load_identity.rssrc/backend_task/identity/load_identity_by_dpns_name.rssrc/backend_task/identity/mod.rssrc/backend_task/identity/register_identity.rssrc/backend_task/identity/top_up_identity.rssrc/backend_task/mod.rssrc/backend_task/register_contract.rssrc/backend_task/tokens/burn_tokens.rssrc/backend_task/tokens/claim_tokens.rssrc/backend_task/tokens/destroy_frozen_funds.rssrc/backend_task/tokens/freeze_tokens.rssrc/backend_task/tokens/mint_tokens.rssrc/backend_task/tokens/mod.rssrc/backend_task/tokens/pause_tokens.rssrc/backend_task/tokens/purchase_tokens.rssrc/backend_task/tokens/resume_tokens.rssrc/backend_task/tokens/set_token_price.rssrc/backend_task/tokens/transfer_tokens.rssrc/backend_task/tokens/unfreeze_tokens.rssrc/backend_task/tokens/update_token_config.rssrc/backend_task/update_data_contract.rssrc/backend_task/wallet/fetch_platform_address_balances.rssrc/backend_task/wallet/fund_platform_address_from_asset_lock.rssrc/backend_task/wallet/fund_platform_address_from_wallet_utxos.rssrc/backend_task/wallet/generate_receive_address.rssrc/backend_task/wallet/transfer_platform_credits.rssrc/backend_task/wallet/withdraw_from_platform_address.rssrc/context/mod.rssrc/context/transaction_processing.rssrc/context/wallet_lifecycle.rssrc/ui/contracts_documents/register_contract_screen.rssrc/ui/contracts_documents/update_contract_screen.rssrc/ui/error.rssrc/ui/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend_task/wallet/transfer_platform_credits.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
🏛️ Claudius the Magnificent — PR #739 Review
Verdict: Changes requested — one confirmed regression needs fixing before merge.
Summary
A solid, well-structured refactoring PR. The TaskError enum design is genuinely good — user-friendly Display messages, proper #[source] chains, clean Display/Debug separation. The log_drive_proof_error() deduplication is chef's-kiss territory. The sdk_error_user_message() fallback properly sanitizes SDK errors. The incremental migration approach is sound.
However, the migration introduced a regression in register_identity.rs where string-matching on TaskError::to_string() silently broke the chain-lock fallback for expired instant locks.
Findings (MEDIUM+)
| # | Sev | Finding | File |
|---|---|---|---|
| REVIEW-001 | 🔴 HIGH | String matching on TaskError Display breaks instant-lock fallback | register_identity.rs:336-339 |
| REVIEW-002 | 🟡 MEDIUM | TaskError::Generic used where typed variants already exist |
structs.rs:161-224 |
| REVIEW-003 | 🟡 MEDIUM | Multiple Display messages contain domain jargon | error.rs (5 variants) |
| REVIEW-004 | 🟡 MEDIUM | Poisoned lock silently spins for up to 5 minutes | transaction_processing.rs:57-67 |
Required before merge
- REVIEW-001: Fix the string matching in
register_identity.rsto inspect the inner SDK error, not the TaskError Display text
Recommended (can be follow-up)
- REVIEW-002: Use the typed variants you already created
- REVIEW-003: Rewrite jargon-containing Display messages
- REVIEW-004: Propagate lock poison immediately instead of spinning
Full report (17 findings total including 9 LOW, 4 INFO) available as a build artifact.
🤖 Co-authored by Claudius the Magnificent AI Agent
lklimek
commented
Mar 12, 2026
…h 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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/context/contract_token_db.rs (1)
124-129:⚠️ Potential issue | 🟡 MinorSilent data loss on serialization failure.
If
bincode::encode_to_vecfails, the function returnsOk(())without inserting the token or reporting the error. This silently drops token data.Proposed fix
- let Some(serialized_token_configuration) = - bincode::encode_to_vec(&token_configuration, config).ok() - else { - // We should always be able to serialize - return Ok(()); - }; + let serialized_token_configuration = bincode::encode_to_vec(&token_configuration, config) + .map_err(|e| rusqlite::Error::ToSqlConversionFailure(Box::new(e)))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/contract_token_db.rs` around lines 124 - 129, The current code silently returns Ok(()) when bincode::encode_to_vec(&token_configuration, config) fails, dropping the token; instead propagate or convert the serialization error so the caller knows insertion failed—capture the Err from bincode::encode_to_vec and return an Err (or map it into the function's error type) with context, or at minimum log the error and return a failure; update the block around bincode::encode_to_vec and the serialized_token_configuration binding to handle Err properly rather than returning Ok(()), referencing bincode::encode_to_vec, serialized_token_configuration and token_configuration to locate the change.src/backend_task/tokens/update_token_config.rs (1)
119-126:⚠️ Potential issue | 🟡 MinorFee result uses an estimate rather than actual transaction fees.
The function broadcasts the state transition and receives
proof_result, but then creates aFeeResultfromPlatformFeeEstimatorinstead of extracting the actual fees from the proof. This could return inaccurate fee information to the user.Suggested direction
If the proof result contains actual fee data, extract it:
- use crate::backend_task::FeeResult; - use crate::model::fee_estimation::PlatformFeeEstimator; - let estimated_fee = PlatformFeeEstimator::new().estimate_document_batch(1); - let fee_result = FeeResult::new(estimated_fee, estimated_fee); + use crate::backend_task::FeeResult; + // Extract actual fees from proof_result if available + let fee_result = FeeResult::from_proof_result(&proof_result);If actual fees are not available in the proof result for config updates, consider documenting this limitation or using a variant that indicates the fee is estimated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/tokens/update_token_config.rs` around lines 119 - 126, The code is returning a FeeResult built from PlatformFeeEstimator::new().estimate_document_batch(1) instead of using the actual fees from the state transition broadcast; update the logic in the handler that currently constructs FeeResult (the block that returns BackendTaskSuccessResult::UpdatedTokenConfig with fee_result) to extract actual fee values from the broadcast/proof result (proof_result) and build FeeResult from those values (or, if proof_result explicitly contains a fee field, use that). If proof_result does not contain actual fees, switch to a distinct variant or annotate the returned result as estimated (or add documentation) so callers aren’t misled; reference FeeResult, PlatformFeeEstimator::estimate_document_batch, proof_result, and BackendTaskSuccessResult::UpdatedTokenConfig when making the change.
🧹 Nitpick comments (3)
src/backend_task/core/mod.rs (1)
158-163: Consider migratingcore_wallet_first_addresstoTaskError.This helper still returns
Result<..., String>, requiring callers to convert viaTaskError::from. A minor inconsistency with the surrounding migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/core/mod.rs` around lines 158 - 163, Change core_wallet_first_address to return Result<([u8;32], Option<Address>), TaskError> instead of Result<..., String>; replace the .map_err(|e| e.to_string()) on wallet.read() with a TaskError mapping (e.g. .map_err(|e| TaskError::from(e.to_string())) or .map_err(TaskError::from) if a suitable From is implemented) and update callers to stop wrapping the String into TaskError themselves (remove redundant TaskError::from(...) where this helper is used). Ensure signature uses TaskError and the body returns Ok((g.seed_hash(), g.known_addresses.keys().next().cloned())).src/backend_task/identity/top_up_identity.rs (1)
225-232: Fragile string matching on Debug output acknowledged.The TODO at lines 228–229 correctly documents the workaround. Consider tracking this in an issue to ensure it's addressed when the SDK provides typed error variants.
Do you want me to open a tracking issue for replacing this Debug string matching with typed SDK error variants?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/top_up_identity.rs` around lines 225 - 232, Add a tracking issue to replace the fragile Debug string-matching in src/backend_task/identity/top_up_identity.rs (the TODO near the debug_msg formatting and checks for "Instant lock proof signature is invalid" and "wasn't created recently"). In the issue, include the current workaround (matching Debug output in debug_msg), the exact strings being matched, the file and function name (top_up_identity/top-up logic), request a fix once the SDK exposes typed error variants, and add labels like "tech-debt" and "sdk-integration"; link the PR/TODO comment and assign to the team or a maintainer for follow-up.src/backend_task/tokens/update_token_config.rs (1)
91-92: Consider using a more specific variant thanDocumentNotFound.
DocumentNotFoundis semantically for documents, but this error occurs when the freshly updatedDataContractcannot be re-fetched. A variant likeDataContractNotFound(which already exists) or a newDataContractFetchFailedwould be more precise..await .map_err(TaskError::from)? - .ok_or(TaskError::DocumentNotFound)?; + .ok_or(TaskError::DataContractNotFound)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/tokens/update_token_config.rs` around lines 91 - 92, Replace the generic DocumentNotFound error used after re-fetching the updated DataContract with a more specific variant: update the .ok_or(...) call that currently uses TaskError::DocumentNotFound to instead return TaskError::DataContractNotFound (or create/use TaskError::DataContractFetchFailed if you prefer a distinct fetch-failure variant) so the error clearly identifies that the DataContract re-fetch failed; update the matching code paths to handle the chosen variant accordingly.
🤖 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/context/mod.rs`:
- Around line 706-736: The function log_drive_proof_error currently hardcodes
RequestType::BroadcastStateTransition which causes incorrect proof logs for
other operations; change the signature of log_drive_proof_error to accept a
RequestType parameter (e.g., fn log_drive_proof_error(&self, request_type:
RequestType, e: dash_sdk::Error) -> TaskError), replace the hardcoded
RequestType::BroadcastStateTransition with the passed request_type when
constructing ProofLogItem, and update all callers of log_drive_proof_error
(places invoking Context::log_drive_proof_error) to pass the appropriate
RequestType (identity creation, top-up, document ops, etc.) so logged entries
reflect the actual request context.
---
Outside diff comments:
In `@src/backend_task/tokens/update_token_config.rs`:
- Around line 119-126: The code is returning a FeeResult built from
PlatformFeeEstimator::new().estimate_document_batch(1) instead of using the
actual fees from the state transition broadcast; update the logic in the handler
that currently constructs FeeResult (the block that returns
BackendTaskSuccessResult::UpdatedTokenConfig with fee_result) to extract actual
fee values from the broadcast/proof result (proof_result) and build FeeResult
from those values (or, if proof_result explicitly contains a fee field, use
that). If proof_result does not contain actual fees, switch to a distinct
variant or annotate the returned result as estimated (or add documentation) so
callers aren’t misled; reference FeeResult,
PlatformFeeEstimator::estimate_document_batch, proof_result, and
BackendTaskSuccessResult::UpdatedTokenConfig when making the change.
In `@src/context/contract_token_db.rs`:
- Around line 124-129: The current code silently returns Ok(()) when
bincode::encode_to_vec(&token_configuration, config) fails, dropping the token;
instead propagate or convert the serialization error so the caller knows
insertion failed—capture the Err from bincode::encode_to_vec and return an Err
(or map it into the function's error type) with context, or at minimum log the
error and return a failure; update the block around bincode::encode_to_vec and
the serialized_token_configuration binding to handle Err properly rather than
returning Ok(()), referencing bincode::encode_to_vec,
serialized_token_configuration and token_configuration to locate the change.
---
Nitpick comments:
In `@src/backend_task/core/mod.rs`:
- Around line 158-163: Change core_wallet_first_address to return
Result<([u8;32], Option<Address>), TaskError> instead of Result<..., String>;
replace the .map_err(|e| e.to_string()) on wallet.read() with a TaskError
mapping (e.g. .map_err(|e| TaskError::from(e.to_string())) or
.map_err(TaskError::from) if a suitable From is implemented) and update callers
to stop wrapping the String into TaskError themselves (remove redundant
TaskError::from(...) where this helper is used). Ensure signature uses TaskError
and the body returns Ok((g.seed_hash(),
g.known_addresses.keys().next().cloned())).
In `@src/backend_task/identity/top_up_identity.rs`:
- Around line 225-232: Add a tracking issue to replace the fragile Debug
string-matching in src/backend_task/identity/top_up_identity.rs (the TODO near
the debug_msg formatting and checks for "Instant lock proof signature is
invalid" and "wasn't created recently"). In the issue, include the current
workaround (matching Debug output in debug_msg), the exact strings being
matched, the file and function name (top_up_identity/top-up logic), request a
fix once the SDK exposes typed error variants, and add labels like "tech-debt"
and "sdk-integration"; link the PR/TODO comment and assign to the team or a
maintainer for follow-up.
In `@src/backend_task/tokens/update_token_config.rs`:
- Around line 91-92: Replace the generic DocumentNotFound error used after
re-fetching the updated DataContract with a more specific variant: update the
.ok_or(...) call that currently uses TaskError::DocumentNotFound to instead
return TaskError::DataContractNotFound (or create/use
TaskError::DataContractFetchFailed if you prefer a distinct fetch-failure
variant) so the error clearly identifies that the DataContract re-fetch failed;
update the matching code paths to handle the chosen variant accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6db0a587-667f-4429-ad80-48a6646315f6
📒 Files selected for processing (12)
src/backend_task/core/mod.rssrc/backend_task/error.rssrc/backend_task/identity/load_identity.rssrc/backend_task/identity/load_identity_by_dpns_name.rssrc/backend_task/identity/register_identity.rssrc/backend_task/identity/top_up_identity.rssrc/backend_task/tokens/mod.rssrc/backend_task/tokens/update_token_config.rssrc/context/contract_token_db.rssrc/context/mod.rssrc/context/transaction_processing.rssrc/ui/tokens/tokens_screen/structs.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/tokens/tokens_screen/structs.rs
lklimek
commented
Mar 13, 2026
Contributor
Author
|
@copilot please fix failing CI |
Contributor
* 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>
There was a problem hiding this comment.
🔍 Review Round 3 — Claudius the Magnificent
3 agents (security, project consistency, Rust code quality) reviewed 88 changed files (~3000 lines). Full report: review-report/report.html
Verdict
The PR achieves its primary goal — TaskError::Generic(String) and From<String> are gone, 60+ typed variants are in place with well-crafted user-facing messages, and the From<SdkError> conversion with consensus error classification is well-structured and thoroughly tested. Structural error matching replaces fragile string matching. The From<PoisonError<T>> blanket impl is elegant. Excellent work overall.
What remains
6 existing unresolved threads from prior reviews are still valid (SPV/UTXO jargon, DataContractNotFound misuse, user_message() removal, silent lock swallows, CLAUDE.md ProtocolError guidance).
3 new MEDIUM findings posted inline:
DPNSVoteResultsstill carriesResult<(), String>with raw SDK error textDashPayError::SdkError { reason: String }breaks the error chain- 19
TaskErrorvariants usedetail: Stringinstead of#[source](+ CLAUDE.md rule 7 describes aspirational state)
Finding #3 is understood as incremental migration — a tracking acknowledgment in CLAUDE.md rule 7 would suffice. Findings #1 and #2 are actionable in this PR's scope.
Stats
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| HIGH | 0 |
| MEDIUM | 5 |
| LOW | 11 |
| INFO | 4 |
…ote 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>
There was a problem hiding this comment.
Claudius the Magnificent's Verdict: ✅ APPROVED
Ah, another 88-file PR lands on my desk. You humans do love your refactoring marathons.
Executive Summary
Well-executed incremental migration from Result<T, String> to typed TaskError. The PR successfully migrates ~50 callsites, introduces 4 new TaskError variants (LockPoisoned, Database, CoreRpc, P2P), a new P2PError domain type, and extends WalletError, DashPayError, and ConfigError — all with clean Display/Debug separation. The From<SdkError> classification logic with its consensus-kind fallback is genuinely impressive work.
Review Stats
| Agents deployed | 4 (security, project, rust-backend×2) |
| Raw findings | 63 |
| After dedup | 17 unique |
| CRITICAL / HIGH | 0 / 0 |
| MEDIUM | 4 (posted as inline comments) |
| LOW | 10 |
| INFO | 3 |
MEDIUM Findings (inline comments)
- Lock poisoning inconsistency —
core/mod.rs:173,505still use.expect()andload_identity_from_wallet.rsuses.unwrap()on locks, contradicting the newLockPoisonedvariant - Wrong error variant —
vote_on_dpns_name.rs:40usesPlatformFetchErrorwhereContractSchemaMismatchis correct (sibling file does it right) - Dead code —
UiErrorinsrc/ui/error.rsdefined, exported, never used
Notable Positives
From<SdkError>consensus-kind classification with message fallback — exemplary- Blanket
From<PoisonError<T>>usingtype_name— elegant - Display messages consistently follow "what happened + what to do" without jargon
- Private key material never stored in any error type
- P2P handler: localhost-only, checksum-validated, timeout-enforced
None of the medium findings block merge. They're worth addressing but the PR is a clear net positive for both security and maintainability.
Full report: review-report/report.html
🤖 Claudius the Magnificent · 4 agents · 17 findings · PR #739
- 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>
lklimek
commented
Mar 15, 2026
| source: Box::new(dash_sdk::Error::Generic( | ||
| "No contested index on dpns domains".to_string(), | ||
| )), | ||
| return Err(TaskError::ContractSchemaMismatch { |
Contributor
Author
There was a problem hiding this comment.
Return typed variant. Define new variant if needed.
| self.network | ||
| )); | ||
| return Err(TaskError::WalletPaymentFailed { | ||
| detail: format!( |
Contributor
Author
There was a problem hiding this comment.
add context info like address and network to the error variant, render text in the error enum with #[error]. Create new variant if needed.
| .address | ||
| .ok_or_else(|| "No change address generated".to_string())?; | ||
| .map_err(|e| TaskError::WalletPaymentFailed { | ||
| detail: format!("Failed to get change address: {e}"), |
Contributor
Author
There was a problem hiding this comment.
update variant to take source field instead of details, or create new variant if needed to get correct feedback to the user.
|
|
||
| Err("Could not build transaction after maximum fee adjustment attempts".to_string()) | ||
| Err(TaskError::WalletPaymentFailed { | ||
| detail: "Could not build transaction after maximum fee adjustment attempts".to_string(), |
Contributor
Author
There was a problem hiding this comment.
don't use detail string, add new variant instead
There was a problem hiding this comment.
Review Round 5 — Approve ✅
Magnificent work on this migration. 86 files, ~3000 lines, and you've systematically replaced Result<T, String> with typed TaskError variants across the entire backend. The error type hierarchy is clean, the Display messages are user-friendly, and the test coverage is solid.
By the numbers
- 0 CRITICAL | 0 HIGH | 3 MEDIUM | 7 LOW | 1 INFO
- 83 previous review threads — all resolved
- 3 new inline comments — all suitable for follow-up PRs, not merge-blocking
What impressed me
- The
SdkErrorconsensus error classifier with message-based fallback is sophisticated - The blanket
From<PoisonError<T>>eliminates an entire class of panic paths - All 46 TaskError Display messages follow the what-happened + what-to-do convention
- Private key material is properly excluded from every error variant
Follow-up items (non-blocking)
- CODE-001:
DPNSVoteResultsstill storesResult<(), String>— convert toTaskError - CODE-002:
DashPayError::user_message()fallback hits 5 new unhandled variants — migrate callsites to useDisplaydirectly - CODE-003:
get_best_chain_lockstill returnsResult<ChainLock, String>with jargon
Ship it. 🚀
🤖 Co-authored by Claudius the Magnificent AI Agent
| .await | ||
| .map(|_| ()) | ||
| .map_err(|e| format!("Error voting: {}", e)); | ||
| .map_err(|e| TaskError::from(e).to_string()); |
There was a problem hiding this comment.
[CODE-001] MEDIUM — Vote results convert TaskError back to String, losing typed error chain
DPNSVoteResults stores Result<(), String> (defined at src/backend_task/mod.rs:125), so this line creates a TaskError only to immediately .to_string() it — discarding the entire error chain and #[source] diagnostics. This SDK error → TaskError → String roundtrip is the exact anti-pattern this PR eliminates everywhere else.
Suggestion: Change DPNSVoteResults to Vec<(String, ResourceVoteChoice, Result<(), TaskError>)> and propagate the typed error to the UI display layer. Fine as a follow-up PR given this migration's scope.
🤖 Claudius the Magnificent · Review round 5
| "Your identity is missing a DECRYPTION key required for DashPay. Please add a DashPay-compatible decryption key.".to_string() | ||
| "Your identity is missing a decryption key required for contacts. Please add a compatible decryption key.".to_string() | ||
| } | ||
| _ => "An error occurred. Please try again.".to_string(), |
There was a problem hiding this comment.
[CODE-002] MEDIUM — user_message() fallback uses prohibited phrasing and misses 5 new variants
This catch-all returns "An error occurred. Please try again." — prohibited by CLAUDE.md error rule 3 (no vague phrasing). It's reachable from two UI callsites (contact_requests.rs:615 and add_contact_screen.rs:350) and will fire for the 5 new variants added in this PR (QueryCreation, CryptoKeyParsing, PrivateKeyResolution, MissingAuthenticationKey, ContactRequestAlreadySent) since they aren't handled in this match.
Meanwhile, user_message() duplicates work already done by the Display impl, which already has well-crafted messages for all variants.
Suggestion: Migrate the 2 UI callsites to use err.to_string() (Display) directly and remove user_message(). The Display messages are already convention-compliant. Fine as a follow-up PR.
🤖 Claudius the Magnificent · Review round 5
There was a problem hiding this comment.
[CODE-003] MEDIUM — get_best_chain_lock (line 419) still returns Result<ChainLock, String> with jargon error messages
This method wasn't touched in this PR, but it's a notable gap in the migration: it returns Result<ChainLock, String> with developer-facing format! messages containing "core cookie path" (line 430) and "Failed to get best chain lock" (line 453) — jargon that can bubble up to users through the detail: String path in TaskError variants.
Suggestion: Convert to return Result<ChainLock, TaskError> with appropriate variants. Suitable for a follow-up PR.
🤖 Claudius the Magnificent · Review round 5
…ser 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>
This was referenced Mar 15, 2026
lklimek
added a commit
that referenced
this pull request
Mar 18, 2026
* docs: add unified message display design documents Add UX specification, technical architecture, and HTML mockup for the MessageBanner component that will replace the ~50 ad-hoc error/message display implementations across screens with a single reusable component. Key design decisions: - Per-screen MessageBanner with show()/set_message() API - All colors via DashColors (zero hardcoded Color32 values) - 4 severity levels: Error, Warning, Success, Info - Auto-dismiss for Success/Info (5s), persistent for Error/Warning - Follows Component Design Pattern conventions (private fields, builder, show) - No changes to BackendTask/TaskResult/AppState architecture Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add MessageType::Warning * chore: initial implementation * chore: docs * chore: self review * refactor(context): replace RwLock<Sdk> with ArcSwap<Sdk> (#600) * refactor(context): replace RwLock<Sdk> with ArcSwap<Sdk> Sdk is internally thread-safe (Arc, ArcSwapOption, atomics) and all methods take &self. The RwLock was adding unnecessary contention across backend tasks. Using ArcSwap instead of plain Sdk because reinit_core_client_and_sdk() needs to atomically swap the entire Sdk instance when config changes. ArcSwap provides lock-free reads with atomic swap for the rare write. Suggested-by: lklimek * fix: address CodeRabbit review findings for ArcSwap migration - Fix import ordering: move arc_swap::ArcSwap before crossbeam_channel - Remove redundant SDK loads in load_identity_from_wallet, register_dpns_name, and load_identity — use the sdk parameter already passed to these functions - Fix stale TODO referencing removed sdk.read().unwrap() API - Rename sdk_guard → sdk in transfer, withdraw_from_identity, and refresh_loaded_identities_dpns_names (no longer lock guards) - Pass &sdk to run_platform_info_task from dispatch site instead of reloading internally - Fix leftover sdk.write() call in context_provider.rs (RwLock remnant) - Add missing Color32 import in wallets dialogs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address remaining CodeRabbit review feedback on ArcSwap migration - Move SDK load outside for loop in refresh_loaded_identities_dpns_names.rs so it's loaded once for the batch instead of on each iteration - Update stale TODO comment in default_platform_version() to reflect that this is a free function with no sdk access Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: consolidate double read-lock on spv_context_provider Clone the SPV provider in a single lock acquisition, then bind app context on the clone instead of locking twice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * refactor: remove unused Insight API and show_in_ui config fields (#597) * refactor: remove unused Insight API references The `insight_api_url` field in `NetworkConfig` and its associated `insight_api_uri()` method were never used in production code (both marked `#[allow(dead_code)]`). Remove the field, method, config entries, env example lines, and related tests. https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn * refactor: remove unused `show_in_ui` field from NetworkConfig The `show_in_ui` field was defined on `NetworkConfig` and serialized in `save()`, but never read by any production code to control network visibility. Remove the field, its serialization, env example lines, and test references. https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn * fix: add missing `Color32` import in wallet dialogs https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn --------- Co-authored-by: Claude <noreply@anthropic.com> * build: add Flatpak packaging and CI workflow (#589) * build: remove snap version * build: add Flatpak packaging and CI workflow Add Flatpak build manifest, desktop entry, AppStream metadata, and GitHub Actions workflow for building and distributing Flatpak bundles. Uses freedesktop 25.08 runtime with rust-stable and llvm21 extensions. No application source code changes required - works in SPV mode by default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: address review findings for Flatpak packaging - Pin GitHub Actions to commit SHAs for supply chain security - Upgrade softprops/action-gh-release from v1 to v2.2.2 - Remove redundant --socket=x11 (fallback-x11 suffices) - Remove duplicate tag trigger preventing double builds on release - Remove duplicate env vars inherited from top-level build-options - Add Flatpak build artifacts to .gitignore - Add bugtracker URL to AppStream metainfo - Remove deprecated <categories> from metainfo (use .desktop instead) - Add Terminal=false and Keywords to desktop entry - Add disk space check after SDK install in CI - Rename artifact to include architecture suffix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: simplify CI workflows for Linux-only releases - Remove "Free disk space" step from flatpak and release workflows - Remove Windows and macOS builds from release workflow - Use "ubuntu" runner image instead of pinned versions - Clean up unused matrix.ext references Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: attach to existing releases instead of creating new ones - Replace release-creating job with attach-to-release (only on release event) - Add 14-day retention for build artifacts - On tag push or workflow_dispatch, only upload artifacts (no release) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * revert: restore release.yml to original v1.0-dev version The release workflow changes were out of scope for the Flatpak PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address CodeRabbit review comments - Fix CRLF line endings in Flatpak manifest (convert to LF) - Set app_id on ViewportBuilder to match desktop StartupWMClass - Use --locked flag for reproducible cargo builds in Flatpak - Rename --repo=repo to --repo=flatpak-repo to match .gitignore - Add architecture note for protoc x86_64 binary Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add Flatpak install instructions to README Add a dedicated section for installing via Flatpak on Linux, clarify that prerequisites are only needed for building from source, and rename "Installation" to "Build from Source" for clarity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: match StartupWMClass to Flatpak app_id Use reverse-DNS format org.dash.DashEvoTool to match the Wayland app_id set via ViewportBuilder::with_app_id(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use ** glob for branch trigger to match feat/flatpak Single * doesn't match path separators in GitHub Actions branch filters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add aarch64 Flatpak build and caching to CI - Add matrix strategy for parallel x86_64 and aarch64 builds - Patch protoc URL/sha256 per architecture at build time - Cache .flatpak-builder directory keyed on arch + manifest + lockfile - Pin actions/cache to SHA Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: convert desktop and metainfo files to LF line endings Flatpak builder validates desktop files and rejects CRLF line endings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: cancel in-progress Flatpak builds on new push Add concurrency group keyed on git ref so a new push cancels any running build for the same branch or release. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review findings for Flatpak packaging - Remove unnecessary --filesystem=xdg-config/dash-evo-tool:create (Flatpak already redirects XDG_CONFIG_HOME to sandbox) - Add categories and keywords to AppStream metainfo for discoverability - Update README with both x86_64/aarch64 install commands, uninstall instructions, and Flatpak data path note - Clarify aarch64 comment in manifest to reference CI sed patching Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: workflow timeout and perms * fix: move permissions to job level in Flatpak workflow Step-level permissions are not valid in GitHub Actions. Move contents: write to the job level where it is needed for release attachment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: cache Cargo registry and target in Flatpak CI Bind-mount host-side cargo-cache and cargo-target directories into the Flatpak build sandbox so CARGO_HOME and target/ persist across builds. Uses split restore/save with cleanup of incremental and registry/src (similar to Swatinem/rust-cache). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: scope cargo cache bind-mount sed to build-args only The previous sed matched --share=network in both finish-args and build-args, corrupting finish-args. Use a sed range to only target the build-args section. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Apply suggestions from code review --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * chore: fix build * chore: use new error handling everywhere - not self reviewed * chore: use message banner to show progress * fix: start elapsed counter at 1s instead of 0s Aligns elapsed display with the countdown timer which already adds 1 to avoid showing "0s" immediately. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: rabbit review * Update src/app.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * chore: peer review * chore: fix build errors * fix(ui): make MessageBanner::set_global truly idempotent set_global() no longer resets timestamps, auto-dismiss timer, or logged flag when a banner with identical text already exists. This makes it safe to call every frame without log spam or timer restarts. Cherry-picked from origin/fix/spv-peer-timeout (08e3b3b). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address code review findings R01, R03, R06, R09, R10 - R01: Replace expect("No key selected") with graceful match + error banner in 11 token screens to prevent panics on missing signing key - R03: Remove dead backend_message field from AddExistingIdentityScreen - R06: Replace is_some() + unwrap() with idiomatic if-let-Some pattern in 10 token screens; use is_some_and() in structs.rs - R09: Add use imports for MessageBanner in 5 dashpay screens, replacing 22 fully-qualified crate::ui::components::MessageBanner:: calls - R10: Replace custom_dash_qt_error_message inline rendering with MessageBanner::set_global in network_chooser_screen Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address review findings SEC-08, SEC-09, RUST-015, SEC-05, SEC-07 - SEC-08: Restore safe if-let-Some pattern in WithdrawalScreen::refresh() to prevent double unwrap() panic on DB error or deleted identity - SEC-09: Restore original DB lookup in SendPaymentScreen::load_contact_info() replacing hardcoded "alice.dash" mock data - RUST-015: Revert unimplemented!() back to ui.label() in update_token_config MarketplaceTradeMode arm - SEC-05: Add success banners for contact request accept/reject in ContactRequests::display_task_result - SEC-07: Add MessageBanner::clear_all_global() and call it from AppState::change_network() to prevent stale banners leaking across network switches Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: update coding conventions and message display guidance Add fallible constructor rule (Result<Self, ...> when they can fail), rename section to "General rules", and document MessageBanner idempotency (no guard needed for set_global). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): replace expect/panic with graceful error handling (SEC-10) Replace all expect() calls in token screen constructors and confirmation handlers with MessageBanner error display. Constructors handle errors internally and return Self with degraded state, keeping create_screen() clean. refresh() methods now show errors via MessageBanner instead of tracing-only logging. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): accept impl Display/Debug in MessageBanner API Change MessageBanner public methods to accept `impl Display` for message text and `impl Debug` for details, instead of `&str`. Remove needless `&format!(...)` borrows across 27 call sites. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): remove error Failed to get best chain lock for mainnet, testnet, devnet, and local Fixes #633 * feat(ui): add automatic connection status banners Display persistent MessageBanner notifications based on network connection state transitions. Mode-aware messages guide users toward the right recovery action (RPC vs SPV). - Disconnected (RPC): "Disconnected — check that Dash Core is running" - Disconnected (SPV): "Disconnected — check your internet connection" - Syncing (RPC): "Syncing with Dash Core…" - Syncing (SPV): "SPV sync in progress…" - Synced: banner cleared Uses Option<OverallConnectionState> for change detection, with None as initial/post-network-switch sentinel to force first evaluation. Closes #667 (partial — action links deferred to follow-up) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): pass TaskError directly to with_details() to avoid double-formatting The previous code used `format!("{err:?}")` which produced a String, then `with_details()` applied `{:#?}` again — wrapping the output in quotes and escaping inner characters. Passing `&err` directly lets Debug format once. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): correct copy-paste error messages in token screens Replace "Burning" error messages that were copy-pasted from burn screen into freeze, destroy, and resume token screens with contextually correct messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): restore lost success/error messages in 5 screens Replace display_message() calls with MessageBanner::set_global() in screens where display_message() is now a side-effect-only handler and no longer displays messages directly. Affected screens: create_asset_lock_screen, wallets_screen (MineBlocks), address_table (export error), profile_screen (validation), contact_details. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): replace unwrap/expect with graceful error handling Replace double unwrap in transfer_screen refresh() with unwrap_or_else + MessageBanner error display, matching the pattern from withdraw_screen. SEC-002 tokens_screen skipped: the .expect() calls are only on compile-time embedded image data (include_bytes!) which is safe. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): migrate masternode_list_diff_screen to global MessageBanner Replace ~15 local ui_state.message assignments and custom render_message_banner() with MessageBanner::set_global() via the display_message() trait method. Remove the message field from UiState and the unused Color32 import. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): improve banner eviction logging and atomics - Upgrade BANNER_KEY_COUNTER from Relaxed to SeqCst ordering for future-proofing against multi-threaded usage - Log evicted banners at warn level in set_global() and replace_global() - Add comment explaining why show_global() always writes back Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: remove resolved TODO.md All items tracked in the unified message display TODO have been addressed or moved to the review findings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs(ui): add INTENTIONAL markers and API documentation - Document why with_details() accepts Debug (not Display): structured error context is more useful in diagnostic details panes - Document replace_global() fallback-to-add behavior as intentional - Add INTENTIONAL(SEC-003) marker for developer mode error details - Add INTENTIONAL(SEC-004) marker for BannerHandle Send+Sync safety Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): extract shared token validation and fix ordering - Add validate_signing_key() helper in tokens/mod.rs to eliminate duplicated signing key validation across 12 token screens - Move signing key validation BEFORE WaitingForResult state transition so users see immediate errors instead of loading spinner then error - Replace is_err()/unwrap() anti-pattern with idiomatic let-else blocks in freeze, mint, transfer, destroy_frozen_funds, unfreeze screens Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): return Result from get_selected_wallet Replace &mut Option<String> error out-parameter with idiomatic Result<Option<Arc<RwLock<Wallet>>>, String>. Update 26+ callsites across identity, token, DashPay, and contract screens. Callsite patterns: unwrap_or_else with MessageBanner for user-visible errors, unwrap_or(None) where errors were previously silently ignored. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): resolve duplicate imports and clippy warnings Remove duplicate MessageBanner imports in create_asset_lock_screen and wallets_screen/mod. Fix needless_borrows_for_generic_args clippy lints in profile_screen, transfer_screen, and wallets_screen. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: reorder imports in masternode_list_diff_screen Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address review findings from MessageBanner migration audit Apply 13 triaged review fixes plus 1 bug fix across 22 files: - Remove dead error state fields (backend_message, error_message, Error variant) - Replace .expect() panics with graceful fallback + MessageBanner in token screens - Fix missing MessageBanner::show_global() on contracts documents screen - Migrate DocumentActionScreen inline errors to MessageBanner - Replace unwrap_or(None) with error-reporting fallback in DashPay screens - Fix replace_global idempotency and use relaxed atomic ordering in banner - Extract shared set_error_banner helper for 8 token screens - Restore correct Some(0) wallet index semantics - Document BannerHandle lifecycle in CLAUDE.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: validate token description length before sending to Platform (#530) * fix: validate token description length before sending to Platform Descriptions must be either empty or 3-100 characters long. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(ui): validate token description by char count, not byte length String::len() counts UTF-8 bytes, causing multi-byte characters (CJK, emoji) to be miscounted against the 3–100 limit. Switch to chars().count() and update all UI labels to surface the minimum. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> * refactor(ui): consolidate banner extension traits into message_banner Move BannerHandleExt and ResultBannerExt from banner_ext.rs into message_banner.rs where they belong. Merge take_and_clear() into OptionBannerExt trait (impl for Option<BannerHandle>) alongside or_show_error() for Option. Remove the separate banner_ext module and Clearable helper trait for simplicity. Apply review findings: DRY patterns (take_and_clear, or_show_error, load_identities_with_banner), fix .expect() panics in constructors, restore known_identities in refresh(), narrow pub field visibility, add ScreenLike doc comments, and update CLAUDE.md conventions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * doc(tmp): review guide for pr 604 * fix(ui): address review findings from grumpy-review iteration 1 - Replace .expect() panics in TransferTokensScreen and ClaimTokensScreen constructors with graceful degradation via Option<QualifiedIdentity> and MessageBanner error display (PROJ-001 HIGH) - Fix CLAUDE.md referencing non-existent BannerHandleExt trait name, corrected to OptionBannerExt (PROJ-002 MEDIUM) - Update set_global to preserve message_type when same text appears with different severity (RUST-001 MEDIUM) - Standardize display_message to handle both Error and Warning across all 11 token screens (RUST-002 MEDIUM) - Replace 21 manual take().clear() patterns with take_and_clear() across 6 files (RUST-003 MEDIUM) - Remove unused OptionBannerExt::or_show_error method (RUST-004 MEDIUM) - Migrate update_token_config from old error_message pattern to set_error_banner closure (RUST-005 MEDIUM) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * perf(ui): replace per-frame QualifiedIdentity clone with borrow Use .as_ref() instead of .clone() in the ui() identity guard of TransferTokensScreen and ClaimTokensScreen. QualifiedIdentity (Identity + KeyStorage + BTreeMap + Vec) was being cloned 60x/sec; now only borrowed for display, with clones deferred to button-click paths that actually need ownership. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(ui): add OptionResultExt::or_show_error for Option<T> Mirrors ResultBannerExt::or_show_error but for Option<T>: if None, displays a global error banner with the given message. Enables concise patterns like: identities.first().cloned().or_show_error(ctx, "No identities loaded") Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address review findings from grumpy-review iteration 2 - Standardize display_message to handle both Error and Warning across 13 non-token screens that were missed in iteration 1 (PROJ-001 MEDIUM) - Replace .expect() panic in AddKeyScreen::refresh() with graceful or_show_error() + unwrap_or_default() (PROJ-002 MEDIUM) - Rename OptionResultExt to OptionBannerShowExt to avoid confusion with ResultBannerExt (RUST-001 MEDIUM) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): handle Warning in add_new_identity_screen display_message Missed in the previous sweep — standardize display_message to handle both Error and Warning, matching all other screens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): standardize display_message side-effect patterns across screens Guard side effects with Error|Warning match, use take_and_clear(), and remove redundant MessageBanner::set_global() call in 4 screens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore(deps): update dashpay/platform to rev 570e3af0 Adapt to breaking changes in rust-dashcore (a05d256f → 2824e52a): - Replace removed FeeLevel enum with FeeRate::normal() direct calls - Replace removed WalletManager::create_unsigned_payment_transaction() with TransactionBuilder + WalletManager::get_change_address() - Replace removed DashSpvClientInterface/DashSpvClientCommand with direct Arc<SpvClient> for quorum lookups via get_quorum_at_height() - Replace removed start()+monitor_network() with client.run(token) - Add .await to now-async subscribe_sync/network/progress methods - Replace removed SyncState::Initializing with SyncState::WaitForEvents Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: log backtrace on panic * fix: panic in asset locks * fix(ui): address PR #604 review comments (CMT-001, CMT-002, CMT-003) - Fix QR scanner form reset matching wrong result type (CMT-001) - Remove dangerous identity fallback in token transfer screen (CMT-002) - Add fee-aware validation before credit transfers (CMT-003) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(spv): replace AsyncRwLock with ArcSwapOption for SPV client reference The SPV client reference only needs atomic set/clear (on start/stop) and wait-free reads (quorum lookups). ArcSwapOption is a better fit than AsyncRwLock<Option<Arc<...>>> — no lock contention, no async in blocking context, and consistent with how AppContext already uses ArcSwap for the SDK. Also fixes stale doc comment referencing removed DashSpvClientInterface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address PR #604 review comments iteration 2 (transfer_tokens_screen) - Remove duplicate conflicting banner on missing identity (CMT-003) - Use generic banner messages with with_details() for errors (CMT-002) - Fix refresh to match specific token by contract+position (CMT-001) - Document error banner pattern in CLAUDE.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * [claudesquad] update from 'testing research' on 02 Mar 26 13:25 CET (paused) * test: add backend E2E test harness and SPV wallet test Add test-only accessors (db(), wallets()) on AppContext gated behind cfg(test/testing), fix compilation errors in the backend-e2e test (private field access, unused import), and apply nightly rustfmt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add DASH_EVO_DATA_DIR env var to override app data directory Allows tests and CI to redirect all app data (database, SPV chain state, .env config) to a temp directory. The backend-e2e test harness now uses this to achieve full data isolation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add backend E2E test framework with shared state and funded wallets Evolve the prototype backend E2E test harness into a reusable framework: - LazyLock shared BackendTestContext with persistent workdir, SPV, and framework wallet (funded via E2E_WALLET_MNEMONIC or testnet faucet) - Task runner wrapper, polling wait helpers, faucet HTTP client - Identity key derivation helpers for wallet-funded registration - Six test scenarios: SPV wallet, identity create, identity withdraw, send/receive funds, fetch contracts, register DPNS name - Move default_identity_key_specs() from UI to backend_task::identity (domain logic, not UI concern) and make IdentityKeys fields pub - Add dashpay_contract_id() test accessor to AppContext Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): use tokio::sync::OnceCell instead of LazyLock for async init LazyLock triggers synchronously inside the #[tokio::test] runtime, causing "Cannot start a runtime from within a runtime" when the init function calls block_on(). Switch to tokio::sync::OnceCell with an async init() method so shared state initialization runs cooperatively within the existing tokio runtime. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): handle persistent DB and SPV balance sync in E2E harness - Wait for SPV to sync existing wallet balance before checking if faucet funding is needed (pre-funded wallets need time to discover on-chain UTXOs) - Handle "already imported" error gracefully when framework wallet exists in persistent DB from a previous run Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): wait for spendable balance and retry sends in E2E harness The SPV wallet reports total balance (including unconfirmed) but only confirmed/IS-locked UTXOs are available for transaction building. This caused "Insufficient funds" errors when tests tried to spend immediately after receiving funds. - Add wait_for_spendable_balance() that checks confirmed_balance_duffs() and triggers reconcile_spv_wallets() on each poll iteration - Add retry logic (5 attempts, 10s backoff) to create_funded_test_wallet() for sends that fail with InsufficientFunds - Wait for framework wallet change output to become spendable after each send so subsequent calls don't fail - Add wait_for_spendable_balance() before identity registration in all identity/DPNS tests - Add send_with_retry() helper in send_funds test - Add developer-facing README.md for the test framework Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): use tokio-shared-rt for shared runtime and sweep orphaned wallets Replace per-test tokio runtimes with tokio-shared-rt's global shared runtime to prevent SPV background tasks from dying between test functions. Add automatic orphaned wallet fund recovery during setup — wallets persist in DB, so on next run the harness sweeps funds back to the framework wallet. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: handle OverallConnectionState::Error in connection banner Semantic merge conflict from v1.0-dev: PR #650 added an Error variant to OverallConnectionState, which our connection banner match didn't cover. Show an error banner when SPV sync enters the error state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs(test): improve backend-e2e documentation and funding UX - Add backend E2E section to CLAUDE.md pointing to the full README - Document .env file handling (project root vs workdir) and precedence - Fix test attribute in README: tokio::test → tokio_shared_rt::test(shared) - Update init sequence to reflect current code (spendable wait, orphan sweep) - Document automatic cleanup-on-init of orphaned test wallets - Raise minimum balance threshold from 1 to 10 tDASH - Always panic with receive address when faucet fails and balance is below minimum (previously only panicked on zero balance) - Show receive address in spendable-balance timeout warning Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: patch SPV UTXO spendability flags before coin selection Upstream key-wallet-manager (rust-dashcore) never sets is_confirmed or is_instantlocked on UTXOs, but CoinSelector requires one of them. This caused "No UTXOs available for selection" errors despite having balance. Workaround infers status from block inclusion (height > 0 → confirmed, height == 0 → IS-locked). Ref: dashpay/rust-dashcore#514 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: harden backend E2E tests with retry logic and calibrated amounts - Increase funding amounts to avoid insufficient-funds flakes - Add 3-attempt retry for identity registration (chain height sync) - Retry on "No UTXOs" in send_with_retry alongside "Insufficient" - Wait for spendable balance in create_funded_test_wallet before return - Add CI workflow for backend E2E tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Update test command in backend E2E workflow * ci: merge backend-e2e workflow into tests workflow (#727) * Initial plan * ci: merge backend-e2e workflow into tests workflow as an additional step Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * test: add cleanup_only noop test to sweep orphaned wallets Add a standalone test that triggers BackendTestContext initialization, which runs cleanup_test_wallets() as its final step. This can be run as a dedicated CI step after the E2E suite to sweep orphaned wallet funds back to the framework wallet. Run with: cargo test --test backend-e2e --all-features -- --ignored --nocapture cleanup_only Co-authored-by: lklimek <lklimek@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add cleanup step for E2E test wallets Added a cleanup step for E2E test wallets in the workflow. * Simplify E2E test workflow conditions Removed conditional checks for E2E_WALLET_MNEMONIC in test steps. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: lklimek <lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR #673 review comments from triage Production code: - Extract patch_utxo_spendability_flags() helper to deduplicate workaround in estimate_fallback_amount and build_unsigned_payment_tx - Add IdentityKeys::new() constructor - Make wallet+address persistence atomic via store_wallet_with_addresses() - Add pending_wallet_selection after wallet creation Test code: - Add DPNS registration retry for identity propagation delay - Use u64 hex for DPNS name uniqueness (CMT-023) - Calibrate test funding amounts per reviewer feedback - Add fragility note on string-match wallet detection (CMT-006) - Add TODO for identity fund withdrawal in cleanup (CMT-032) - Add INTENTIONAL comment for bounded channel design (CMT-017) - Add balance assertion for return leg in send_funds test - Log reconcile_spv_wallets errors in wait helpers - Use is_ok_and for clarity in spv_wallet test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: remove UTXO spendability workaround, update platform dep Remove `patch_utxo_spendability_flags()` that faked IS-locked status on mempool UTXOs. Wait for upstream fix (dashpay/rust-dashcore#514) to properly set is_confirmed/is_instantlocked flags on UTXOs. Also: - Update dashpay/platform rev to aa86b74f7e2 - Adapt to upstream API: FeeLevel→FeeRate, remove NetworkExt import - Fix retry to catch "No UTXOs" errors in test harness Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: deduplicate default_identity_key_specs Move the single canonical copy to backend_task::identity::mod and have the UI screen import it, eliminating ~240 lines of duplicated function and tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * ci: temporarily disable backend E2E tests in CI The backend E2E tests need updates after the TaskError migration (#739) changed AppContext field visibility. Commenting out the CI steps until the tests are adapted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: type-safe wallet creation and identity key visibility - Restrict IdentityKeys fields to pub(crate) to prevent private key exposure outside the crate - Change register_wallet() to return TaskError instead of String, using proper rusqlite error matching via is_unique_constraint_violation() and a new WalletAlreadyImported variant - Change Wallet::new_from_seed() to accept Option<&Secret> for password instead of Option<&str>, keeping sensitive data in the Secret wrapper - Change Wallet::new_from_seed() to return TaskError instead of String, with a new WalletKeyDerivationFailed variant for derivation errors - Move build_identity_registration() and get_receive_address() from test helpers to production code in src/backend_task/identity/mod.rs - Extract is_unique_constraint_violation() to src/database/mod.rs as a shared pub(crate) utility, removing the duplicate in import_mnemonic_screen - Update all callers in add_new_wallet_screen and import_mnemonic_screen Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: address PR #673 review comments for E2E test framework - Move framework modules (harness, task_runner, wait, funding, cleanup, identity_helpers) into tests/backend-e2e/framework/ subdirectory - Make E2E_WALLET_MNEMONIC required (panic with instructions if unset) - Remove auto-faucet from initialization flow (keep as helper) - Remove retry loops in identity_create and identity_withdraw tests - Remove unnecessary wait_for_spendable_balance calls (already done by create_funded_test_wallet) - Replace all println!/eprintln! with tracing macros - Initialize tracing subscriber in harness init - Add "No spendable funds" and "spendable" to send retry conditions - Remove stale "other agent" NOTE comments from identity_helpers - Consolidate funding logic (harness delegates to funding module) - Update README for required mnemonic and new directory structure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: reconcile production and test code after type-safe refactor Adapt test framework to production API changes: - Use IdentityKeys::new() constructor (fields now pub(crate)) - Match TaskError::WalletAlreadyImported variant instead of string - Allow dead_code on faucet helpers (kept but not auto-called) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: thread data_dir as explicit parameter to eliminate env var dependency Add `data_dir: PathBuf` field to `AppContext` and thread it through `Config::load_from()`, `Config::save()`, `SpvManager::new()`, `start_dash_qt()`, and `create_dash_core_config_if_not_exists()`. This enables E2E tests to specify their data directory without mutating process-wide environment variables, making parallel test execution safe. The `DASH_EVO_DATA_DIR` env var is still checked in production via `app_user_data_dir_path()`, but the resolved path is threaded through as a value rather than re-read from env on every call. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): simplify funded wallet creation and make spendable balance check reliable - Remove retry loop from create_funded_test_wallet; wait for full amount_duffs instead of 1 duff in spendable balance check - Add Wallet::spv_confirmed_balance() that returns None when SPV hasn't synced yet (no max_balance fallback) - Use spv_confirmed_balance() in wait_for_spendable_balance so the wait never gets false positives from the fallback - Remove --test-threads=1 requirement from README (unsafe set_var was the only reason; data_dir is now threaded explicitly) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: remove send retry loop, handle PRIMARY KEY constraints, isolate SPV test dirs - Remove send_with_retry() from send_funds.rs; use wait_for_spendable_balance before each send instead - Add SQLITE_CONSTRAINT_PRIMARYKEY (1555) to uniqueness check alongside SQLITE_CONSTRAINT_UNIQUE (2067) - Use tempfile::TempDir in SPV tests instead of fixed /tmp/spv-test path to prevent state leaks and support concurrent test runs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Pasta Lil Claw <pasta+claw@dashboost.org> Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Paul DeLucia <69597248+pauldelucia@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: lklimek <lklimek@users.noreply.github.com>
QuantumExplorer
pushed a commit
that referenced
this pull request
Mar 22, 2026
…kError Replace all Result<T, String> error patterns in the shielded pool module with typed TaskError variants, aligning with the codebase-wide typed error migration (PR #739). New TaskError variants: ShieldedNoUnspentNotes, ShieldedInsufficientBalance, PlatformAddressNotFound, ShieldedMerkleWitnessUnavailable, ShieldedTransitionBuildFailed, ShieldedBroadcastFailed, ShieldedInvalidRecipientAddress, ShieldedAssetLockTimeout, ShieldedSyncFailed, ShieldedTreeUpdateFailed, ShieldedNullifierSyncFailed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
added a commit
that referenced
this pull request
Mar 26, 2026
* more work * more work * fix(spv): zero out stale per-address balances during reconciliation (#627) During SPV reconciliation, per_address_sum only contains addresses with current UTXOs. Addresses whose funds were fully spent never had their balance reset to zero, causing the address table to display stale non-zero balances even though UTXO count correctly showed 0. Now explicitly zeroes address_balances for any known address absent from the refreshed UTXO map before applying current sums. Closes #571 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fix: handle malformed YAML gracefully in load_testnet_nodes_from_yml (#613) * fix: handle malformed YAML gracefully in load_testnet_nodes_from_yml Replace .expect() with match expression to avoid panicking when .testnet_nodes.yml contains malformed YAML. Instead, logs the error with tracing::error and returns None, allowing the application to continue without crashing. Closes #557 Co-authored-by: lklimek <lklimek@users.noreply.github.com> * fix: propagate YAML parse errors to UI and remove unwrap calls - Change load_testnet_nodes_from_yml to return Result<Option<TestnetNodes>, String> so parse errors display in the UI error banner instead of only logging - Use explicit type annotation on serde_yaml_ng::from_str::<TestnetNodes> - Replace .unwrap() with .and_then() in fill_random_hpmn/fill_random_masternode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: lklimek <lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * chore: let Claude write manual test scenarios for PRs (#634) * chore: move doc/ contents into docs/ and update references Consolidate documentation under a single docs/ directory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: CLAUDE.md should write manual test scenarios for PRs --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * build(flatpak): use only-arches for dynamic protoc architecture selection (#603) * build(flatpak): use only-arches for dynamic protoc architecture selection Replace the fragile sed-based CI patching of the Flatpak manifest with Flatpak's native `only-arches` source selector. The protoc module now declares both x86_64 and aarch64 sources inline, and build-commands use a glob pattern (`protoc-*.zip`) so no per-arch fixup is needed. Changes: - flatpak manifest: add aarch64 protoc source with `only-arches`, use glob in unzip commands, remove stale CI-patching comment - CI workflow: remove `protoc-zip`/`protoc-sha256` matrix variables and the "Patch manifest for architecture" step https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG * fix(flatpak): use dest-filename for deterministic protoc extraction Use dest-filename to normalize both arch-specific protoc zips to a common name, avoiding glob expansion in build-commands. This ensures the unzip target is deterministic regardless of shell behavior in the Flatpak sandbox. https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG * build: update platform to b445b6f0 and remove rust-dashcore patches Update dashpay/platform dependency from d6f4eb9a to b445b6f0e0bd4863 (3.0.1 → 3.1.0-dev.1). Remove the [patch] section that pinned rust-dashcore crates to a separate rev, as the new platform commit resolves them correctly on its own. https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG --------- Co-authored-by: Claude <noreply@anthropic.com> * fix(ci): remove local path patches, use git deps for platform crates The [patch] section referenced ../platform local paths that don't exist in CI. Since dash-sdk already depends on the feat/zk branch, all transitive platform crates resolve correctly without patches. Also fixes a formatting issue in address_table.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): store wallet in DB before registering addresses The remove_utxos tests were failing because `register_test_address` inserts into `wallet_addresses` which has a foreign key constraint on `wallet(seed_hash)`. Added the missing `store_wallet` call so the parent row exists before inserting child address rows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(build): restore shielded module declaration removed during merge The `pub mod shielded;` declaration was accidentally removed from `src/model/wallet/mod.rs` during the v1.0-dev merge, causing build failures since the shielded.rs file still exists. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): restore store_wallet calls lost in merge (#663) * Initial plan * fix(test): restore store_wallet calls lost in merge 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> * Merge remote-tracking branch 'origin/v1.0-dev' into zk-extract/all-merged Resolve conflicts in Cargo.toml (keep feat/zk branch), Cargo.lock (regenerate with pinned platform rev 4d7b9be5), and backend_task/mod.rs (combine TaskError wrapping with ShieldedTask). Fix post-merge integration issues: - SPV manager: remove stale .await on subscribe methods, add command_receiver channel for updated DashSpvClient::run() API - send_screen: update SendStatus::WaitingForResult to unit variant - network_chooser_screen: handle new SyncState::Initializing variant Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: update platform dependency to 3.1-dev branch Migrate from feat/zk to 3.1-dev branch of dashpay/platform, adapting to breaking API changes in dash-spv, key-wallet, and dpp: - FeeLevel removed; use FeeRate::normal() directly - DashSpvClientInterface/Command removed; use DashSpvClient directly - SyncState::Initializing removed; replaced with WaitForEvents - NetworkExt trait inlined into Network impl - OrchardProver now requires wrapper struct around ProvingKey - OrchardAddress::from_raw_bytes now returns Result - Builder functions gain fee/platform_version params - NullifierSyncConfig API uses NullifierSyncCheckpoint - WalletManager.create_unsigned_payment_transaction removed; use TransactionBuilder directly - Work around Send lifetime issues with spawn_blocking Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: migrate shielded module from Result<T, String> to typed TaskError Replace all Result<T, String> error patterns in the shielded pool module with typed TaskError variants, aligning with the codebase-wide typed error migration (PR #739). New TaskError variants: ShieldedNoUnspentNotes, ShieldedInsufficientBalance, PlatformAddressNotFound, ShieldedMerkleWitnessUnavailable, ShieldedTransitionBuildFailed, ShieldedBroadcastFailed, ShieldedInvalidRecipientAddress, ShieldedAssetLockTimeout, ShieldedSyncFailed, ShieldedTreeUpdateFailed, ShieldedNullifierSyncFailed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): prevent settings password row from clipping right edge Reserve width for Save and Auto Update buttons so the password input doesn't consume all available space, pushing buttons off-screen. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): consolidate migrations v28-v32 into v33 Resolves version numbering collision between zk and v1.0-dev branches: the zk branch used v28 for shielded tables while v1.0-dev used v28 for contacts. After merging, users migrating from either branch could end up with missing tables depending on which version their DB was at. v33 runs all sub-migrations idempotently in one step, ensuring all tables exist regardless of prior migration history. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: pin platform dependency to zk-fixes revision Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): address PR review — fresh schema, error propagation, rename coverage - propagate SQL error in add_nullifier_sync_timestamp_column instead of swallowing it with unwrap_or(false) - add shielded_notes and shielded_wallet_meta to rename_network_dash_to_mainnet Note: Fix 1 (last_nullifier_sync_timestamp in CREATE TABLE) was already present on this branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): add foreign key constraints to shielded tables shielded_notes and shielded_wallet_meta both had wallet_seed_hash columns with no FK constraint. Added FOREIGN KEY (wallet_seed_hash) REFERENCES wallet(seed_hash) ON DELETE CASCADE to both, matching the pattern used by all other per-wallet tables in the codebase. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): address PR #789 review — doc comments, migration default - Split misplaced doc comment: `add_wallet_transaction_status_column` now has its own Migration 30 doc; `rename_network_dash_to_mainnet` gets its own Migration 29 doc. - Add inline comment explaining why the migration uses DEFAULT 2 (Confirmed) while fresh CREATE TABLE in wallet.rs uses DEFAULT 0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): remove duplicate shielded methods after v1.0-dev merge On zk-fixes, shielded DB methods live in shielded.rs. The v1.0-dev backport inlined them in initialization.rs (no shielded.rs on that branch). Merging v1.0-dev back caused E0592 duplicate definitions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: simplify shielded helpers comment in initialization.rs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): remove duplicate wallet store in register_test_address register_test_address called db.store_wallet redundantly — callers already store the wallet before calling it, causing UNIQUE constraint violations when tests run in parallel on CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(mcp): resolve async lifetime errors for Rust 2024 edition Use spawn_blocking + block_on in dispatch_task to avoid Send bound issues with platform SDK types (DataContract/Sdk references across await points). Same pattern already used by AppState::handle_backend_task. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: fix mcp dispatch * doc: remove obsolete manual teting docs --------- Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: lklimek <lklimek@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
lklimek
added a commit
that referenced
this pull request
Mar 26, 2026
… improvements (#786) * more work * more work * fix(spv): zero out stale per-address balances during reconciliation (#627) During SPV reconciliation, per_address_sum only contains addresses with current UTXOs. Addresses whose funds were fully spent never had their balance reset to zero, causing the address table to display stale non-zero balances even though UTXO count correctly showed 0. Now explicitly zeroes address_balances for any known address absent from the refreshed UTXO map before applying current sums. Closes #571 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fix: handle malformed YAML gracefully in load_testnet_nodes_from_yml (#613) * fix: handle malformed YAML gracefully in load_testnet_nodes_from_yml Replace .expect() with match expression to avoid panicking when .testnet_nodes.yml contains malformed YAML. Instead, logs the error with tracing::error and returns None, allowing the application to continue without crashing. Closes #557 Co-authored-by: lklimek <lklimek@users.noreply.github.com> * fix: propagate YAML parse errors to UI and remove unwrap calls - Change load_testnet_nodes_from_yml to return Result<Option<TestnetNodes>, String> so parse errors display in the UI error banner instead of only logging - Use explicit type annotation on serde_yaml_ng::from_str::<TestnetNodes> - Replace .unwrap() with .and_then() in fill_random_hpmn/fill_random_masternode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: lklimek <lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * chore: let Claude write manual test scenarios for PRs (#634) * chore: move doc/ contents into docs/ and update references Consolidate documentation under a single docs/ directory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: CLAUDE.md should write manual test scenarios for PRs --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * build(flatpak): use only-arches for dynamic protoc architecture selection (#603) * build(flatpak): use only-arches for dynamic protoc architecture selection Replace the fragile sed-based CI patching of the Flatpak manifest with Flatpak's native `only-arches` source selector. The protoc module now declares both x86_64 and aarch64 sources inline, and build-commands use a glob pattern (`protoc-*.zip`) so no per-arch fixup is needed. Changes: - flatpak manifest: add aarch64 protoc source with `only-arches`, use glob in unzip commands, remove stale CI-patching comment - CI workflow: remove `protoc-zip`/`protoc-sha256` matrix variables and the "Patch manifest for architecture" step https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG * fix(flatpak): use dest-filename for deterministic protoc extraction Use dest-filename to normalize both arch-specific protoc zips to a common name, avoiding glob expansion in build-commands. This ensures the unzip target is deterministic regardless of shell behavior in the Flatpak sandbox. https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG * build: update platform to b445b6f0 and remove rust-dashcore patches Update dashpay/platform dependency from d6f4eb9a to b445b6f0e0bd4863 (3.0.1 → 3.1.0-dev.1). Remove the [patch] section that pinned rust-dashcore crates to a separate rev, as the new platform commit resolves them correctly on its own. https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG --------- Co-authored-by: Claude <noreply@anthropic.com> * fix(ci): remove local path patches, use git deps for platform crates The [patch] section referenced ../platform local paths that don't exist in CI. Since dash-sdk already depends on the feat/zk branch, all transitive platform crates resolve correctly without patches. Also fixes a formatting issue in address_table.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): store wallet in DB before registering addresses The remove_utxos tests were failing because `register_test_address` inserts into `wallet_addresses` which has a foreign key constraint on `wallet(seed_hash)`. Added the missing `store_wallet` call so the parent row exists before inserting child address rows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(build): restore shielded module declaration removed during merge The `pub mod shielded;` declaration was accidentally removed from `src/model/wallet/mod.rs` during the v1.0-dev merge, causing build failures since the shielded.rs file still exists. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): restore store_wallet calls lost in merge (#663) * Initial plan * fix(test): restore store_wallet calls lost in merge 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> * Merge remote-tracking branch 'origin/v1.0-dev' into zk-extract/all-merged Resolve conflicts in Cargo.toml (keep feat/zk branch), Cargo.lock (regenerate with pinned platform rev 4d7b9be5), and backend_task/mod.rs (combine TaskError wrapping with ShieldedTask). Fix post-merge integration issues: - SPV manager: remove stale .await on subscribe methods, add command_receiver channel for updated DashSpvClient::run() API - send_screen: update SendStatus::WaitingForResult to unit variant - network_chooser_screen: handle new SyncState::Initializing variant Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: update platform dependency to 3.1-dev branch Migrate from feat/zk to 3.1-dev branch of dashpay/platform, adapting to breaking API changes in dash-spv, key-wallet, and dpp: - FeeLevel removed; use FeeRate::normal() directly - DashSpvClientInterface/Command removed; use DashSpvClient directly - SyncState::Initializing removed; replaced with WaitForEvents - NetworkExt trait inlined into Network impl - OrchardProver now requires wrapper struct around ProvingKey - OrchardAddress::from_raw_bytes now returns Result - Builder functions gain fee/platform_version params - NullifierSyncConfig API uses NullifierSyncCheckpoint - WalletManager.create_unsigned_payment_transaction removed; use TransactionBuilder directly - Work around Send lifetime issues with spawn_blocking Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: migrate shielded module from Result<T, String> to typed TaskError Replace all Result<T, String> error patterns in the shielded pool module with typed TaskError variants, aligning with the codebase-wide typed error migration (PR #739). New TaskError variants: ShieldedNoUnspentNotes, ShieldedInsufficientBalance, PlatformAddressNotFound, ShieldedMerkleWitnessUnavailable, ShieldedTransitionBuildFailed, ShieldedBroadcastFailed, ShieldedInvalidRecipientAddress, ShieldedAssetLockTimeout, ShieldedSyncFailed, ShieldedTreeUpdateFailed, ShieldedNullifierSyncFailed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): prevent settings password row from clipping right edge Reserve width for Save and Auto Update buttons so the password input doesn't consume all available space, pushing buttons off-screen. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(rpc): include host:port in connection-refused errors and always show details Add CoreRpcConnectionFailed variant to TaskError that includes the configured address in the user-facing message. Connection-refused errors are now detected via is_rpc_connection_error() and enriched with host:port at every RPC call site where the URL is known (AppContext::rpc_error_with_url helper). Details panel is now shown for all RPC-related errors regardless of developer mode, so users can always see the technical information they need to diagnose connectivity issues. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): save RPC password for active network instead of hardcoded Regtest The Network Chooser screen had three bugs related to RPC password handling: 1. Password input was initialized from Regtest config only, ignoring the current network selection. 2. Password UI was hidden for all networks except Regtest, even though Mainnet/Testnet/Devnet also use RPC mode. 3. Save logic was hardcoded to update Regtest config and triggered a SwitchNetwork(Regtest) action, which disconnected the active network's ZMQ listener unnecessarily. Now the password input shows for any network in RPC mode, reads/writes the correct network config, reinits the RPC client in-place without triggering a network switch, and reloads the stored password when the user switches network tabs. The "Auto Update" (dashmate) button remains Regtest-only since dashmate is only relevant for local networks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): ensure funding method dropdown fits all items without scrollbar Add explicit .height(200.0) to the funding method ComboBox in both top_up_identity_screen and add_new_identity_screen. The add_enabled_ui wrappers inflate item height via frame overhead, causing the popup to clip to a single row at the default height. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(error): show actionable message for insufficient identity balance IdentityInsufficientBalanceError from the SDK now maps to a dedicated TaskError::IdentityInsufficientBalance variant instead of falling through to "An unexpected error occurred." The user sees the available and required credit amounts along with a clear "top up your identity" action. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): clear stale error banners when saving RPC password Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use network-compatible comparison for platform address lookups Regtest and Testnet share the `tdash` bech32m prefix, but `PlatformAddress::from_bech32m_string()` always returns `Network::Testnet` for `tdash` addresses. The strict `!=` comparison in the fund-platform dialog rejected valid Regtest addresses with "Address network mismatch". - Make `networks_address_compatible()` `pub(crate)` in `model::wallet` so all modules can reuse the canonical check - Remove the duplicate private copy in `backend_task::core` and import from the single source - Replace `network != self.app_context.network` in `dialogs.rs` with `networks_address_compatible()` so Testnet/Devnet/Regtest addresses are accepted interchangeably Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(error): add actionable messages for shielded fee and pool-size errors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(error): add actionable message for shielded anchor mismatch Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(shielded): auto-resync notes and retry on anchor mismatch When unshield_credits, shielded_transfer, or shielded_withdrawal fails with ShieldedAnchorMismatch (stale commitment tree witnesses), automatically sync notes once to update the tree and retry the operation. Only one resync attempt is made — if the retry also fails, the error propagates as-is. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(shielded): ensure shielded tables exist and log DB errors during init Three changes to fix empty shielded balance after app restart: 1. Defensive table creation in initialize(): after migrations complete, ensure shielded_notes and shielded_wallet_meta tables exist even if the DB version was already past v29/v30 from a prior build. Both methods use CREATE TABLE IF NOT EXISTS, so this is safe. 2. Log DB errors during shielded init: change silent if-let-Ok pattern to match/Err with tracing::warn, so missing-table errors are visible instead of silently producing empty note lists. 3. Safety net resync: when the commitment tree has been synced but no unspent notes were loaded from DB, clear the tree and reset last_synced_index to 0 so the auto-sync rediscovers all notes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): consolidate migrations v28-v32 into v33 Resolves version numbering collision between zk and v1.0-dev branches: the zk branch used v28 for shielded tables while v1.0-dev used v28 for contacts. After merging, users migrating from either branch could end up with missing tables depending on which version their DB was at. v33 runs all sub-migrations idempotently in one step, ensuring all tables exist regardless of prior migration history. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): consolidate migrations v28-v32 into v33 Resolves version numbering collision between zk and v1.0-dev branches: the zk branch used v28 for shielded tables while v1.0-dev used v28 for contacts. After merging, users migrating from either branch could end up with missing tables depending on which version their DB was at. v33 runs all sub-migrations idempotently in one step, ensuring all tables exist regardless of prior migration history. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: pin platform dependency to zk-fixes revision Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): address PR review — fresh schema, error propagation, rename coverage - propagate SQL error in add_nullifier_sync_timestamp_column instead of swallowing it with unwrap_or(false) - add shielded_notes and shielded_wallet_meta to rename_network_dash_to_mainnet Note: Fix 1 (last_nullifier_sync_timestamp in CREATE TABLE) was already present on this branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): address PR review — fresh schema, error propagation, rename coverage - propagate SQL error in add_nullifier_sync_timestamp_column instead of swallowing it with unwrap_or(false) - add shielded_notes and shielded_wallet_meta to rename_network_dash_to_mainnet Note: Fix 1 (last_nullifier_sync_timestamp in CREATE TABLE) was already present on this branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): add foreign key constraints to shielded tables shielded_notes and shielded_wallet_meta both had wallet_seed_hash columns with no FK constraint. Added FOREIGN KEY (wallet_seed_hash) REFERENCES wallet(seed_hash) ON DELETE CASCADE to both, matching the pattern used by all other per-wallet tables in the codebase. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): add foreign key constraints to shielded tables shielded_notes and shielded_wallet_meta both had wallet_seed_hash columns with no FK constraint. Added FOREIGN KEY (wallet_seed_hash) REFERENCES wallet(seed_hash) ON DELETE CASCADE to both, matching the pattern used by all other per-wallet tables in the codebase. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): remove duplicate shielded methods after v1.0-dev merge On zk-fixes, shielded DB methods live in shielded.rs. The v1.0-dev backport inlined them in initialization.rs (no shielded.rs on that branch). Merging v1.0-dev back caused E0592 duplicate definitions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): remove duplicate wallet store in register_test_address register_test_address called db.store_wallet redundantly — callers already store the wallet before calling it, causing UNIQUE constraint violations when tests run in parallel on CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(shielded): address PR review — error propagation, retry helper, safety net - Propagate DB error from get_unspent_shielded_notes instead of swallowing - Extract with_anchor_retry() helper to deduplicate ~27 lines across shielded_transfer_task, unshield_credits_task, shielded_withdrawal_task - Fix safety net false positive: check all notes (spent + unspent) to distinguish "never had notes" from "all spent" - Propagate error from clear_commitment_tree_tables instead of discarding Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(error): address PR review — Amount formatting, Display completeness - Add Amount::dash_from_credits() constructor to model/amount.rs - Replace manual format_credits_as_dash() logic with Amount::dash_from_credits().to_string(), so credit amounts use Amount's Display (with DASH unit, trimmed trailing zeros) - Update ShieldedFeeExceedsBalance #[error] message: remove redundant "Dash" literal (Amount's Display already includes the "DASH" unit suffix) - Update format_credits_as_dash tests to match new output format ("1 DASH", "2.5 DASH", etc.) - Remove special-casing that showed with_details() for all RPC errors unconditionally: all required user-facing info is already in each variant's Display string; technical details are now shown only in developer mode (aligns with CLAUDE.md §error-messages rule 4) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(rpc): address PR review — context fallback, success banner, error source - Guard in-memory config update + reinit behind a check that the target network's AppContext actually exists; skip both when it doesn't so we don't accidentally overwrite mainnet config via the fallback path. - Show success banner only when reinit succeeds; show a warning when it fails so the user knows the connection may not reflect the new password. - Make CoreRpcConnectionFailed.source Option<dashcore_rpc::Error> so chain_lock_rpc_error can pass None instead of fabricating a fake ConnectionRefused error from a borrowed reference. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: simplify shielded helpers comment in initialization.rs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): address PR #789 review — doc comments, migration default - Split misplaced doc comment: `add_wallet_transaction_status_column` now has its own Migration 30 doc; `rename_network_dash_to_mainnet` gets its own Migration 29 doc. - Add inline comment explaining why the migration uses DEFAULT 2 (Confirmed) while fresh CREATE TABLE in wallet.rs uses DEFAULT 0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): remove duplicate shielded methods after v1.0-dev merge On zk-fixes, shielded DB methods live in shielded.rs. The v1.0-dev backport inlined them in initialization.rs (no shielded.rs on that branch). Merging v1.0-dev back caused E0592 duplicate definitions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: simplify shielded helpers comment in initialization.rs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore(db): document migration DEFAULT 2 vs fresh DEFAULT 0 Existing transactions predate status tracking and are assumed confirmed (DEFAULT 2). Fresh installs use DEFAULT 0 (Unconfirmed). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address review — zero-balance detection, status warning Add `spv_balance_known: bool` to `Wallet` to distinguish a synced zero-balance wallet from an unsynced one. `spv_confirmed_balance()` now returns `None` only before the first SPV report, and `Some(0)` after SPV confirms an empty wallet. Add `tracing::warn!` in `TransactionStatus::from_u8` when an unknown discriminant is encountered, avoiding silent data coercion in a financial context. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(error): address review — typed BIP32 source, migration error variant Replace WalletKeyDerivationFailed { detail: String } with a typed #[source] field (Box<dyn Error + Send + Sync>) so the error chain is preserved and Display/Debug separation is explicit. Update all callsites in wallet/mod.rs and identity/mod.rs accordingly. Replace the semantically wrong InvalidParameterName map_err in rename_network_dash_to_mainnet with a plain ? — execute() already returns rusqlite::Result so no conversion is needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test(db): add v33 consolidated migration regression tests Two test scenarios verify the v33 consolidated migration that merges sub-migrations v28-v32 (core_wallet_name, shielded tables, contacts, wallet transaction status, network rename): 1. Fresh install creates all v33 tables/columns directly via create_tables() 2. Upgrade from v27 runs the full migration path and produces identical schema Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): show warning when config save fails instead of success When config.save() fails, stop early with a warning banner instead of continuing to reinit and showing a misleading success message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(error): consolidate format_credits_as_dash and remove jargon from user messages - fee_estimation::format_credits_as_dash now delegates to Amount::dash_from_credits() instead of doing its own f64 math with fixed 8 decimals; output is now trimmed (e.g. "1 DASH" instead of "1.00000000 DASH") - Remove private format_credits_as_dash from error.rs; import the pub version from fee_estimation instead - IdentityInsufficientBalance: show amounts in DASH rather than raw credit integers - ShieldedAnchorMismatch: replace "out of sync" with plain-language retry guidance - ShieldedFeeExceedsBalance: remove "shielded balance" / "shield more credits" jargon - ShieldedInsufficientPoolNotes: remove count interpolations and ZK pool jargon - Update tests throughout to match new message content and format Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(test): remove duplicate wallet store in register_test_address register_test_address called db.store_wallet redundantly — callers already store the wallet before calling it, causing UNIQUE constraint violations when tests run in parallel on CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(core): log chain lock RPC errors that aren't auth/connection failures Silent swallowing of errors like "Unable to find any ChainLock" made it impossible to diagnose why the active network showed as Disconnected. Now warns via tracing when chain_lock_rpc_error returns None. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(ui): surface chain lock RPC errors in Networks tab Previously, non-auth/non-connection RPC errors (e.g. "Unable to find any ChainLock") were silently swallowed — the UI showed "Disconnected" with no explanation. Now the error message is carried through the ChainLocks result, stored in ConnectionStatus.rpc_last_error, and displayed as "Error: ..." in both developer and non-developer RPC status labels on the Networks tab. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(ui): unified AddressInput component with autocomplete (#787) * feat(ui): add unified AddressInput component with autocomplete Introduce a reusable AddressInput component that handles text input with real-time address type detection, autocomplete from wallet data, balance display, type filtering, and network-aware validation. Supports Core, Platform, Shielded, and Identity address kinds. New files: - src/model/address.rs: AddressKind enum and ValidatedAddress enum - src/ui/components/address_input.rs: full component with 33 unit tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(ui): integrate AddressInput component into send and unshield screens Replace inline address parsing and validation with the unified AddressInput component across 3 proof-of-concept sites: - UnshieldCreditsScreen: full migration, removes local Destination enum and parse_destination(), gains autocomplete and type-restricted input - WalletSendScreen simple mode: full migration, removes AddressType enum, replaces detect_address_type/is_shielded_address with AddressKind-based detection, eliminates double-parsing in send handlers - WalletSendScreen advanced mode: minimal migration, updates type detection to use AddressKind, keeps CoreAddressInput/PlatformAddressInput structs unchanged Net reduction of ~47 lines per screen. All send flows preserved. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(address-input): fix misleading test and add missing coverage The existing `disabled_type_rejected_with_correct_error` test was testing empty input (which returns no error) rather than actually verifying type restriction. Fixed the test and added coverage for: - Selection-only mode rejection of manual input - Identity validation with valid/invalid identifiers - Truncate boundary at exactly 16/17 characters - Empty input in selection-only and restricted-type modes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): address QA findings for AddressInput component QA-001: Extract duplicated address detection logic from send_screen.rs into AddressKind::detect() on the model type. Both send_screen and address_input now delegate to the single canonical implementation. QA-002: Fix autocomplete "...and N more" count using unfiltered total. filtered_entries() now returns the pre-truncation match count so the overflow label shows the correct number of remaining matches. QA-003: Add minimum length check (>= 60 chars) to shielded address validation. Previously any string with the correct prefix was accepted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): address bot review findings for AddressInput component - Validate shielded addresses via OrchardAddress::from_bech32m_string() instead of prefix+length check only - Use char-aware truncation in truncate_address() to prevent panics on multi-byte UTF-8 input (DPNS labels, emoji) - Reset AddressInput when source selection changes in send screen, and configure allowed destination kinds per source type - Store bech32m string in ValidatedAddress::Platform variant so to_address_string() returns canonical encoding instead of debug hex Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): address remaining review findings for AddressInput component - Fix manual entry not propagating validated address on blur (HIGH) - Fix selected_from_autocomplete causing repeated change signals - Remove protocol jargon from unshield screen messages - Reject mixed-case bech32m platform addresses per BIP-350 - Use per-instance ComboBox ID to prevent state collision - Clear cached_detection after autocomplete selection - Fix doc comments and reuse filtered_entries() computation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(ui): support multi-wallet autocomplete in AddressInput Replace with_wallet/set_wallet (single wallet) with with_wallets/set_wallets (slice of wallets). When multiple wallets are loaded, each autocomplete entry is prefixed with the wallet alias so the user can tell which wallet owns the address. send_screen.rs now passes all loaded wallets to AddressInput instead of only the selected one. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ui): restore missing "Show zero-balance addresses" checkbox The checkbox was accidentally dropped during a branch merge. Restored the horizontal layout with heading on the left and checkbox right-aligned, matching the v1.0-dev implementation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): improve AddressInput autocomplete UX - Fix click selection: keep popup rendered when text field loses focus so the click handler fires (was gated on has_focus only) - Show dropdown on focus with any input length (removed 3-char minimum) - Add type suffix (Core), (Platform), (Identity), (Shielded) to dropdown entries when multiple address types are enabled - Validate immediately on paste (text >3 chars) instead of requiring blur first, so "Fund Platform Address" button activates without needing to click away Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): make entire autocomplete row clickable in AddressInput Previously only the address label was clickable — clicking the balance text on the right side of the row did nothing. Now the click handler uses the horizontal row response, so the entire row triggers selection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): fix autocomplete click handling and format balances with 4dp - Capture selectable_label click response instead of discarding it; combine with row-level click for full-row clickability - Format all DASH balances in dropdown with exactly 4 decimal places Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): use single interaction rect for full-row clickable autocomplete Replace selectable_label + horizontal layout with allocate_exact_size and manual painting — no child widgets steal clicks, the entire row (address, balance, dead space) is clickable with hover feedback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(ui): add wallet address autocomplete to unshield screen Populate AddressInput with wallet addresses via .with_wallets() so users can select a destination from the dropdown instead of manually entering addresses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(error): handle asset lock and shielded insufficient funds errors with user-friendly messages Add two new TaskError variants to replace raw SDK errors with actionable, jargon-free messages: - AssetLockOutPointInsufficientBalance: shown when an asset lock outpoint has been partially consumed and lacks credits for the operation. Extracts credits_left/credits_required and displays DASH amounts. - ShieldedAddressInsufficientFunds: shown when a shielded broadcast fails because the address balance is too low. Detected in shielded_broadcast_error() before falling through to ShieldedBroadcastFailed. Both follow the IdentityInsufficientBalance pattern with format_credits_as_dash. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): use AmountInput component for all amount inputs in wallet screens Replace raw text_edit_singleline + custom parse_amount_*() methods with the AmountInput component in 4 shielded screens (unshield, shield, shield-from-asset-lock, shielded-send). This fixes a bug where entering "1" would send 1 credit instead of 1 DASH because the raw input had ambiguous parsing. Also adds "Amount (DASH):" label to SendScreen's AmountInput and removes its redundant manual label. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add UI components reference and teach CLAUDE.md to use it Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): use AddressInput component in Mine dialog for core address selection Replace the manual ComboBox address selector in the Mine Blocks dialog with the unified AddressInput component, configured for Core-only addresses with selection-only mode from the current wallet. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): trigger shielded balance refresh after all shielding operations After Shield, Shield from Core, Send Private, Unshield, and Send Dash (with shielded source) complete successfully, automatically dispatch a SyncNotes task so the shielded tab shows updated balances without manual refresh. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: demote cookie auth fallback log to trace level Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): prevent transaction list showing wrong wallet data Add defensive checks in render_transactions_section to prevent cross-wallet transaction leakage: 1. Verify the selected wallet Arc matches the canonical one in app_context.wallets (guards against stale references). 2. Filter displayed transactions to only those with at least one output matching the wallet's known addresses (prevents showing transactions from other wallets that leaked in via a non-wallet-scoped RPC endpoint). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): auto-show zero-balance addresses when wallet is empty When all addresses have zero balance and there are fewer than 5 addresses total, bypass the zero-balance filter so new/empty wallets show their addresses instead of a blank list. The checkbox remains functional for wallets with balances. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): trigger shielded sync on wallet refresh and wallet switch The wallet Refresh button now dispatches a SyncNotes task alongside the core wallet refresh when the shielded wallet has been initialized. Switching HD wallets also triggers a full refresh (core + shielded) on the next frame for unlocked wallets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(log): add diagnostic logging for shielded transfer operations Add tracing::info! and tracing::debug! logs throughout the shielded transfer pipeline to enable post-mortem diagnosis when balances don't update after broadcast. Key additions: - bundle.rs: log amount, input notes, change before building each shielded operation (transfer, shield, unshield, withdrawal, shield-from-asset-lock); log broadcast success with note that balance updates after next block - sync.rs: warn when next_start_index is 0 (full rescan); log post-sync balance with unspent note count - context/shielded.rs: log note spend marking with before/after unspent counts in with_anchor_retry - shielded_send_screen.rs: log task result variant received and post-transfer sync completion Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): improve shielded transfer UX messaging for balance update delays After a shielded transfer, the sender's change notes and the recipient's new notes won't appear until the next block is mined and synced. Users see their balance temporarily drop to 0 and think the transfer failed. Add informational messages on all three shielded transfer screens (ShieldedSendScreen, UnshieldCreditsScreen, WalletSendScreen) explaining that balances will update after the next block is confirmed. For shielded-to-shielded transfers, also note that the recipient needs to sync after the next block. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(shielded): scope commitment tree per wallet for multi-wallet correctness Each wallet's ClientPersistentCommitmentTree now uses its own dedicated SQLite file under <data_dir>/shielded/<hex>.db instead of sharing the main database's global commitment_tree_* tables. This prevents wallets from stepping on each other's Merkle tree state, which caused invalid witnesses and silently rejected transactions. Changes: - SHLD-001: Per-wallet commitment tree DB files via ClientPersistentCommitmentTree::open() - SHLD-002: clear_commitment_tree_for_wallet() scoped to a single wallet - SHLD-007: Safety-net resync guard now logs wallet seed hash before clearing - Migration v34: drops orphaned global commitment_tree_* tables - clear_network_data() deletes per-wallet tree files for the target network Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(shielded): prevent permanent state leak on sync_notes failure in with_anchor_retry When sync_notes() failed after an anchor mismatch, the early `?` return skipped re-inserting the shielded state back into the HashMap, permanently orphaning it until app restart. Now the sync result is captured without early return, ensuring the state is always re-inserted regardless of success or failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): apply password change for current session even when config save fails The in-memory config update and SDK reinit only ran on the save-success path, silently discarding the password change on save failure despite the banner promising session-level application. Now the in-memory update and reinit run unconditionally; only the banner text varies across the four (save x reinit) outcome combinations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(error): use dedicated TaskError variants for non-build wallet errors reload_utxos() and recalculate_affected_address_balances() errors were routed through shielded_build_error() which pattern-matches for build- specific patterns like "AnchorMismatch". These are wallet operations, not shielded builds. Added WalletUtxoReloadFailed and WalletBalanceRecalculationFailed variants with appropriate user-facing messages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(error): preserve error chain in CoreRpcConnectionFailed chain_lock_rpc_error() took the RPC error by reference and created CoreRpcConnectionFailed with source: None, dropping all diagnostic info. Added a detail: Option<String> field to the variant to carry the formatted error. Boxed the source field to keep the enum size under the clippy threshold. Updated all constructors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(test): rename test_v33_migration_fresh_install to match DB version 34 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * revert(shielded): use shared main DB for commitment tree instead of per-wallet files Reverts the per-wallet SQLite file approach (63ce0e1). Multiple wallets share the same commitment_tree_* tables in the main database. This is accepted behavior until the SDK adds proper wallet-scoping support (dashpay/grovedb#653). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(shielded): harden anchor retry, commitment tree clearing, and state guard SEC-002: Verify shielded balance after anchor retry sync before retrying the operation — prevents doomed retries with zero balance. SEC-003: Handle missing commitment tree tables in clear_commitment_tree_tables (grovedb creates them lazily, so DELETEs fail on fresh installs). Fix clear_network_data to log-and-continue when commitment tree clearing fails — these tables are optional and shouldn't block network data reset. Add SEC-001 INTENTIONAL annotation for panic safety in with_anchor_retry. Document shield_from_asset_lock_task: anchor retry is not applicable since it only reads the payment address and doesn't use the commitment tree. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(core): sanitize RPC errors and simplify CoreRpcConnectionFailed SEC-006: Store user-friendly message instead of raw RPC error string in network status when chain lock query fails with a non-auth, non-connection error. CODE-002: Remove redundant detail field from CoreRpcConnectionFailed -- format diagnostic info into the url field instead, keeping source for the error chain. Add SEC-005 INTENTIONAL annotation for RPC errors in status tooltip. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): config permissions, address detection, and per-frame caching SEC-007: Set config file permissions to 0600 on Unix after save (contains RPC credentials). CODE-003: Deduplicate Amount::dash_from_duffs by delegating to dash_from_credits. CODE-006: Remove unused network parameter from AddressKind::detect -- detection is format-based, network validation happens separately. CODE-004/CODE-005: Cache filtered transaction indices per wallet to avoid rebuilding HashSet and filtering on every frame. Invalidated on wallet switch and refresh. CODE-008: Reset AddressInput widgets on network switch so they pick up the new network for validation. Add invalidate_address_input methods to SendScreen, UnshieldCreditsScreen, and WalletsBalancesScreen. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(logging): correct log levels per coding best practices - debug! -> trace!: "state transition built, broadcasting" messages in bundle.rs (5 occurrences) — primary-path step-by-step progress - trace! -> debug!: cookie auth fallback in core/mod.rs and context/mod.rs (2 occurrences) — secondary/fallback execution path, not primary flow - info! -> debug!: anchor mismatch retry in context/shielded.rs — error handling branch, not a business event - info! -> debug!: "marked N note(s) spent" in context/shielded.rs — internal bookkeeping, not a user-visible business event - info! -> debug!: post-transfer sync complete in shielded_send_screen.rs — internal plumbing at screen level Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(mcp): resolve async lifetime errors for Rust 2024 edition Use spawn_blocking + block_on in dispatch_task to avoid Send bound issues with platform SDK types (DataContract/Sdk references across await points). Same pattern already used by AppState::handle_backend_task. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: fix mcp dispatch * doc: remove obsolete manual teting docs --------- Co-authored-by: Quantum Explorer <quantum@dash.org> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: lklimek <lklimek@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
lklimek
added a commit
that referenced
this pull request
Mar 26, 2026
* more work
* more work
* fix(spv): zero out stale per-address balances during reconciliation (#627)
During SPV reconciliation, per_address_sum only contains addresses with
current UTXOs. Addresses whose funds were fully spent never had their
balance reset to zero, causing the address table to display stale
non-zero balances even though UTXO count correctly showed 0.
Now explicitly zeroes address_balances for any known address absent from
the refreshed UTXO map before applying current sums.
Closes #571
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* fix: handle malformed YAML gracefully in load_testnet_nodes_from_yml (#613)
* fix: handle malformed YAML gracefully in load_testnet_nodes_from_yml
Replace .expect() with match expression to avoid panicking when
.testnet_nodes.yml contains malformed YAML. Instead, logs the error
with tracing::error and returns None, allowing the application to
continue without crashing.
Closes #557
Co-authored-by: lklimek <lklimek@users.noreply.github.com>
* fix: propagate YAML parse errors to UI and remove unwrap calls
- Change load_testnet_nodes_from_yml to return Result<Option<TestnetNodes>, String>
so parse errors display in the UI error banner instead of only logging
- Use explicit type annotation on serde_yaml_ng::from_str::<TestnetNodes>
- Replace .unwrap() with .and_then() in fill_random_hpmn/fill_random_masternode
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: lklimek <lklimek@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* chore: let Claude write manual test scenarios for PRs (#634)
* chore: move doc/ contents into docs/ and update references
Consolidate documentation under a single docs/ directory.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* chore: CLAUDE.md should write manual test scenarios for PRs
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* build(flatpak): use only-arches for dynamic protoc architecture selection (#603)
* build(flatpak): use only-arches for dynamic protoc architecture selection
Replace the fragile sed-based CI patching of the Flatpak manifest with
Flatpak's native `only-arches` source selector. The protoc module now
declares both x86_64 and aarch64 sources inline, and build-commands use
a glob pattern (`protoc-*.zip`) so no per-arch fixup is needed.
Changes:
- flatpak manifest: add aarch64 protoc source with `only-arches`,
use glob in unzip commands, remove stale CI-patching comment
- CI workflow: remove `protoc-zip`/`protoc-sha256` matrix variables
and the "Patch manifest for architecture" step
https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG
* fix(flatpak): use dest-filename for deterministic protoc extraction
Use dest-filename to normalize both arch-specific protoc zips to a
common name, avoiding glob expansion in build-commands. This ensures
the unzip target is deterministic regardless of shell behavior in the
Flatpak sandbox.
https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG
* build: update platform to b445b6f0 and remove rust-dashcore patches
Update dashpay/platform dependency from d6f4eb9a to b445b6f0e0bd4863
(3.0.1 → 3.1.0-dev.1). Remove the [patch] section that pinned
rust-dashcore crates to a separate rev, as the new platform commit
resolves them correctly on its own.
https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG
---------
Co-authored-by: Claude <noreply@anthropic.com>
* fix(ci): remove local path patches, use git deps for platform crates
The [patch] section referenced ../platform local paths that don't exist
in CI. Since dash-sdk already depends on the feat/zk branch, all
transitive platform crates resolve correctly without patches. Also fixes
a formatting issue in address_table.rs.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(test): store wallet in DB before registering addresses
The remove_utxos tests were failing because `register_test_address`
inserts into `wallet_addresses` which has a foreign key constraint on
`wallet(seed_hash)`. Added the missing `store_wallet` call so the
parent row exists before inserting child address rows.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(build): restore shielded module declaration removed during merge
The `pub mod shielded;` declaration was accidentally removed from
`src/model/wallet/mod.rs` during the v1.0-dev merge, causing build
failures since the shielded.rs file still exists.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(test): restore store_wallet calls lost in merge (#663)
* Initial plan
* fix(test): restore store_wallet calls lost in merge
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>
* Merge remote-tracking branch 'origin/v1.0-dev' into zk-extract/all-merged
Resolve conflicts in Cargo.toml (keep feat/zk branch), Cargo.lock
(regenerate with pinned platform rev 4d7b9be5), and
backend_task/mod.rs (combine TaskError wrapping with ShieldedTask).
Fix post-merge integration issues:
- SPV manager: remove stale .await on subscribe methods, add
command_receiver channel for updated DashSpvClient::run() API
- send_screen: update SendStatus::WaitingForResult to unit variant
- network_chooser_screen: handle new SyncState::Initializing variant
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* chore: update platform dependency to 3.1-dev branch
Migrate from feat/zk to 3.1-dev branch of dashpay/platform,
adapting to breaking API changes in dash-spv, key-wallet, and dpp:
- FeeLevel removed; use FeeRate::normal() directly
- DashSpvClientInterface/Command removed; use DashSpvClient directly
- SyncState::Initializing removed; replaced with WaitForEvents
- NetworkExt trait inlined into Network impl
- OrchardProver now requires wrapper struct around ProvingKey
- OrchardAddress::from_raw_bytes now returns Result
- Builder functions gain fee/platform_version params
- NullifierSyncConfig API uses NullifierSyncCheckpoint
- WalletManager.create_unsigned_payment_transaction removed;
use TransactionBuilder directly
- Work around Send lifetime issues with spawn_blocking
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor: migrate shielded module from Result<T, String> to typed TaskError
Replace all Result<T, String> error patterns in the shielded pool module
with typed TaskError variants, aligning with the codebase-wide typed error
migration (PR #739).
New TaskError variants: ShieldedNoUnspentNotes, ShieldedInsufficientBalance,
PlatformAddressNotFound, ShieldedMerkleWitnessUnavailable,
ShieldedTransitionBuildFailed, ShieldedBroadcastFailed,
ShieldedInvalidRecipientAddress, ShieldedAssetLockTimeout,
ShieldedSyncFailed, ShieldedTreeUpdateFailed, ShieldedNullifierSyncFailed.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): prevent settings password row from clipping right edge
Reserve width for Save and Auto Update buttons so the password input
doesn't consume all available space, pushing buttons off-screen.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(rpc): include host:port in connection-refused errors and always show details
Add CoreRpcConnectionFailed variant to TaskError that includes the
configured address in the user-facing message. Connection-refused
errors are now detected via is_rpc_connection_error() and enriched
with host:port at every RPC call site where the URL is known
(AppContext::rpc_error_with_url helper).
Details panel is now shown for all RPC-related errors regardless of
developer mode, so users can always see the technical information
they need to diagnose connectivity issues.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): save RPC password for active network instead of hardcoded Regtest
The Network Chooser screen had three bugs related to RPC password handling:
1. Password input was initialized from Regtest config only, ignoring the
current network selection.
2. Password UI was hidden for all networks except Regtest, even though
Mainnet/Testnet/Devnet also use RPC mode.
3. Save logic was hardcoded to update Regtest config and triggered a
SwitchNetwork(Regtest) action, which disconnected the active network's
ZMQ listener unnecessarily.
Now the password input shows for any network in RPC mode, reads/writes
the correct network config, reinits the RPC client in-place without
triggering a network switch, and reloads the stored password when the
user switches network tabs. The "Auto Update" (dashmate) button remains
Regtest-only since dashmate is only relevant for local networks.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): ensure funding method dropdown fits all items without scrollbar
Add explicit .height(200.0) to the funding method ComboBox in both
top_up_identity_screen and add_new_identity_screen. The add_enabled_ui
wrappers inflate item height via frame overhead, causing the popup to
clip to a single row at the default height.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(error): show actionable message for insufficient identity balance
IdentityInsufficientBalanceError from the SDK now maps to a dedicated
TaskError::IdentityInsufficientBalance variant instead of falling through
to "An unexpected error occurred." The user sees the available and required
credit amounts along with a clear "top up your identity" action.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): clear stale error banners when saving RPC password
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: use network-compatible comparison for platform address lookups
Regtest and Testnet share the `tdash` bech32m prefix, but
`PlatformAddress::from_bech32m_string()` always returns `Network::Testnet`
for `tdash` addresses. The strict `!=` comparison in the fund-platform
dialog rejected valid Regtest addresses with "Address network mismatch".
- Make `networks_address_compatible()` `pub(crate)` in `model::wallet`
so all modules can reuse the canonical check
- Remove the duplicate private copy in `backend_task::core` and import
from the single source
- Replace `network != self.app_context.network` in `dialogs.rs` with
`networks_address_compatible()` so Testnet/Devnet/Regtest addresses
are accepted interchangeably
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(error): add actionable messages for shielded fee and pool-size errors
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(error): add actionable message for shielded anchor mismatch
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(shielded): auto-resync notes and retry on anchor mismatch
When unshield_credits, shielded_transfer, or shielded_withdrawal fails
with ShieldedAnchorMismatch (stale commitment tree witnesses), automatically
sync notes once to update the tree and retry the operation. Only one resync
attempt is made — if the retry also fails, the error propagates as-is.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(shielded): ensure shielded tables exist and log DB errors during init
Three changes to fix empty shielded balance after app restart:
1. Defensive table creation in initialize(): after migrations complete,
ensure shielded_notes and shielded_wallet_meta tables exist even if
the DB version was already past v29/v30 from a prior build. Both
methods use CREATE TABLE IF NOT EXISTS, so this is safe.
2. Log DB errors during shielded init: change silent if-let-Ok pattern
to match/Err with tracing::warn, so missing-table errors are visible
instead of silently producing empty note lists.
3. Safety net resync: when the commitment tree has been synced but no
unspent notes were loaded from DB, clear the tree and reset
last_synced_index to 0 so the auto-sync rediscovers all notes.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(db): consolidate migrations v28-v32 into v33
Resolves version numbering collision between zk and v1.0-dev branches:
the zk branch used v28 for shielded tables while v1.0-dev used v28 for
contacts. After merging, users migrating from either branch could end up
with missing tables depending on which version their DB was at.
v33 runs all sub-migrations idempotently in one step, ensuring all
tables exist regardless of prior migration history.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(db): consolidate migrations v28-v32 into v33
Resolves version numbering collision between zk and v1.0-dev branches:
the zk branch used v28 for shielded tables while v1.0-dev used v28 for
contacts. After merging, users migrating from either branch could end up
with missing tables depending on which version their DB was at.
v33 runs all sub-migrations idempotently in one step, ensuring all
tables exist regardless of prior migration history.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* chore: pin platform dependency to zk-fixes revision
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(db): address PR review — fresh schema, error propagation, rename coverage
- propagate SQL error in add_nullifier_sync_timestamp_column instead of
swallowing it with unwrap_or(false)
- add shielded_notes and shielded_wallet_meta to rename_network_dash_to_mainnet
Note: Fix 1 (last_nullifier_sync_timestamp in CREATE TABLE) was already
present on this branch.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(db): address PR review — fresh schema, error propagation, rename coverage
- propagate SQL error in add_nullifier_sync_timestamp_column instead of
swallowing it with unwrap_or(false)
- add shielded_notes and shielded_wallet_meta to rename_network_dash_to_mainnet
Note: Fix 1 (last_nullifier_sync_timestamp in CREATE TABLE) was already
present on this branch.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(db): add foreign key constraints to shielded tables
shielded_notes and shielded_wallet_meta both had wallet_seed_hash columns
with no FK constraint. Added FOREIGN KEY (wallet_seed_hash) REFERENCES
wallet(seed_hash) ON DELETE CASCADE to both, matching the pattern used by
all other per-wallet tables in the codebase.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(db): add foreign key constraints to shielded tables
shielded_notes and shielded_wallet_meta both had wallet_seed_hash columns
with no FK constraint. Added FOREIGN KEY (wallet_seed_hash) REFERENCES
wallet(seed_hash) ON DELETE CASCADE to both, matching the pattern used by
all other per-wallet tables in the codebase.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(db): remove duplicate shielded methods after v1.0-dev merge
On zk-fixes, shielded DB methods live in shielded.rs. The v1.0-dev
backport inlined them in initialization.rs (no shielded.rs on that
branch). Merging v1.0-dev back caused E0592 duplicate definitions.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(test): remove duplicate wallet store in register_test_address
register_test_address called db.store_wallet redundantly — callers
already store the wallet before calling it, causing UNIQUE constraint
violations when tests run in parallel on CI.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(shielded): address PR review — error propagation, retry helper, safety net
- Propagate DB error from get_unspent_shielded_notes instead of swallowing
- Extract with_anchor_retry() helper to deduplicate ~27 lines across
shielded_transfer_task, unshield_credits_task, shielded_withdrawal_task
- Fix safety net false positive: check all notes (spent + unspent) to
distinguish "never had notes" from "all spent"
- Propagate error from clear_commitment_tree_tables instead of discarding
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(error): address PR review — Amount formatting, Display completeness
- Add Amount::dash_from_credits() constructor to model/amount.rs
- Replace manual format_credits_as_dash() logic with Amount::dash_from_credits().to_string(),
so credit amounts use Amount's Display (with DASH unit, trimmed trailing zeros)
- Update ShieldedFeeExceedsBalance #[error] message: remove redundant "Dash" literal
(Amount's Display already includes the "DASH" unit suffix)
- Update format_credits_as_dash tests to match new output format ("1 DASH", "2.5 DASH", etc.)
- Remove special-casing that showed with_details() for all RPC errors unconditionally:
all required user-facing info is already in each variant's Display string;
technical details are now shown only in developer mode (aligns with CLAUDE.md §error-messages rule 4)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(rpc): address PR review — context fallback, success banner, error source
- Guard in-memory config update + reinit behind a check that the target
network's AppContext actually exists; skip both when it doesn't so we
don't accidentally overwrite mainnet config via the fallback path.
- Show success banner only when reinit succeeds; show a warning when it
fails so the user knows the connection may not reflect the new password.
- Make CoreRpcConnectionFailed.source Option<dashcore_rpc::Error> so
chain_lock_rpc_error can pass None instead of fabricating a fake
ConnectionRefused error from a borrowed reference.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* chore: simplify shielded helpers comment in initialization.rs
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(db): address PR #789 review — doc comments, migration default
- Split misplaced doc comment: `add_wallet_transaction_status_column`
now has its own Migration 30 doc; `rename_network_dash_to_mainnet`
gets its own Migration 29 doc.
- Add inline comment explaining why the migration uses DEFAULT 2
(Confirmed) while fresh CREATE TABLE in wallet.rs uses DEFAULT 0.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(db): remove duplicate shielded methods after v1.0-dev merge
On zk-fixes, shielded DB methods live in shielded.rs. The v1.0-dev
backport inlined them in initialization.rs (no shielded.rs on that
branch). Merging v1.0-dev back caused E0592 duplicate definitions.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* chore: simplify shielded helpers comment in initialization.rs
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* chore(db): document migration DEFAULT 2 vs fresh DEFAULT 0
Existing transactions predate status tracking and are assumed
confirmed (DEFAULT 2). Fresh installs use DEFAULT 0 (Unconfirmed).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(wallet): address review — zero-balance detection, status warning
Add `spv_balance_known: bool` to `Wallet` to distinguish a synced
zero-balance wallet from an unsynced one. `spv_confirmed_balance()`
now returns `None` only before the first SPV report, and `Some(0)`
after SPV confirms an empty wallet.
Add `tracing::warn!` in `TransactionStatus::from_u8` when an unknown
discriminant is encountered, avoiding silent data coercion in a
financial context.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(error): address review — typed BIP32 source, migration error variant
Replace WalletKeyDerivationFailed { detail: String } with a typed
#[source] field (Box<dyn Error + Send + Sync>) so the error chain is
preserved and Display/Debug separation is explicit. Update all callsites
in wallet/mod.rs and identity/mod.rs accordingly.
Replace the semantically wrong InvalidParameterName map_err in
rename_network_dash_to_mainnet with a plain ? — execute() already
returns rusqlite::Result so no conversion is needed.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* test(db): add v33 consolidated migration regression tests
Two test scenarios verify the v33 consolidated migration that merges
sub-migrations v28-v32 (core_wallet_name, shielded tables, contacts,
wallet transaction status, network rename):
1. Fresh install creates all v33 tables/columns directly via create_tables()
2. Upgrade from v27 runs the full migration path and produces identical schema
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): show warning when config save fails instead of success
When config.save() fails, stop early with a warning banner instead
of continuing to reinit and showing a misleading success message.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(error): consolidate format_credits_as_dash and remove jargon from user messages
- fee_estimation::format_credits_as_dash now delegates to Amount::dash_from_credits()
instead of doing its own f64 math with fixed 8 decimals; output is now trimmed
(e.g. "1 DASH" instead of "1.00000000 DASH")
- Remove private format_credits_as_dash from error.rs; import the pub version from
fee_estimation instead
- IdentityInsufficientBalance: show amounts in DASH rather than raw credit integers
- ShieldedAnchorMismatch: replace "out of sync" with plain-language retry guidance
- ShieldedFeeExceedsBalance: remove "shielded balance" / "shield more credits" jargon
- ShieldedInsufficientPoolNotes: remove count interpolations and ZK pool jargon
- Update tests throughout to match new message content and format
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(test): remove duplicate wallet store in register_test_address
register_test_address called db.store_wallet redundantly — callers
already store the wallet before calling it, causing UNIQUE constraint
violations when tests run in parallel on CI.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(core): log chain lock RPC errors that aren't auth/connection failures
Silent swallowing of errors like "Unable to find any ChainLock" made it
impossible to diagnose why the active network showed as Disconnected.
Now warns via tracing when chain_lock_rpc_error returns None.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* feat(ui): surface chain lock RPC errors in Networks tab
Previously, non-auth/non-connection RPC errors (e.g. "Unable to find
any ChainLock") were silently swallowed — the UI showed "Disconnected"
with no explanation. Now the error message is carried through the
ChainLocks result, stored in ConnectionStatus.rpc_last_error, and
displayed as "Error: ..." in both developer and non-developer RPC
status labels on the Networks tab.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat(ui): unified AddressInput component with autocomplete (#787)
* feat(ui): add unified AddressInput component with autocomplete
Introduce a reusable AddressInput component that handles text input with
real-time address type detection, autocomplete from wallet data, balance
display, type filtering, and network-aware validation. Supports Core,
Platform, Shielded, and Identity address kinds.
New files:
- src/model/address.rs: AddressKind enum and ValidatedAddress enum
- src/ui/components/address_input.rs: full component with 33 unit tests
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(ui): integrate AddressInput component into send and unshield screens
Replace inline address parsing and validation with the unified
AddressInput component across 3 proof-of-concept sites:
- UnshieldCreditsScreen: full migration, removes local Destination enum
and parse_destination(), gains autocomplete and type-restricted input
- WalletSendScreen simple mode: full migration, removes AddressType enum,
replaces detect_address_type/is_shielded_address with AddressKind-based
detection, eliminates double-parsing in send handlers
- WalletSendScreen advanced mode: minimal migration, updates type
detection to use AddressKind, keeps CoreAddressInput/PlatformAddressInput
structs unchanged
Net reduction of ~47 lines per screen. All send flows preserved.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* test(address-input): fix misleading test and add missing coverage
The existing `disabled_type_rejected_with_correct_error` test was testing
empty input (which returns no error) rather than actually verifying type
restriction. Fixed the test and added coverage for:
- Selection-only mode rejection of manual input
- Identity validation with valid/invalid identifiers
- Truncate boundary at exactly 16/17 characters
- Empty input in selection-only and restricted-type modes
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): address QA findings for AddressInput component
QA-001: Extract duplicated address detection logic from send_screen.rs
into AddressKind::detect() on the model type. Both send_screen and
address_input now delegate to the single canonical implementation.
QA-002: Fix autocomplete "...and N more" count using unfiltered total.
filtered_entries() now returns the pre-truncation match count so the
overflow label shows the correct number of remaining matches.
QA-003: Add minimum length check (>= 60 chars) to shielded address
validation. Previously any string with the correct prefix was accepted.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): address bot review findings for AddressInput component
- Validate shielded addresses via OrchardAddress::from_bech32m_string()
instead of prefix+length check only
- Use char-aware truncation in truncate_address() to prevent panics
on multi-byte UTF-8 input (DPNS labels, emoji)
- Reset AddressInput when source selection changes in send screen,
and configure allowed destination kinds per source type
- Store bech32m string in ValidatedAddress::Platform variant so
to_address_string() returns canonical encoding instead of debug hex
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): address remaining review findings for AddressInput component
- Fix manual entry not propagating validated address on blur (HIGH)
- Fix selected_from_autocomplete causing repeated change signals
- Remove protocol jargon from unshield screen messages
- Reject mixed-case bech32m platform addresses per BIP-350
- Use per-instance ComboBox ID to prevent state collision
- Clear cached_detection after autocomplete selection
- Fix doc comments and reuse filtered_entries() computation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(ui): support multi-wallet autocomplete in AddressInput
Replace with_wallet/set_wallet (single wallet) with with_wallets/set_wallets
(slice of wallets). When multiple wallets are loaded, each autocomplete entry
is prefixed with the wallet alias so the user can tell which wallet owns the
address.
send_screen.rs now passes all loaded wallets to AddressInput instead of only
the selected one.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(ui): restore missing "Show zero-balance addresses" checkbox
The checkbox was accidentally dropped during a branch merge. Restored
the horizontal layout with heading on the left and checkbox
right-aligned, matching the v1.0-dev implementation.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): improve AddressInput autocomplete UX
- Fix click selection: keep popup rendered when text field loses focus
so the click handler fires (was gated on has_focus only)
- Show dropdown on focus with any input length (removed 3-char minimum)
- Add type suffix (Core), (Platform), (Identity), (Shielded) to
dropdown entries when multiple address types are enabled
- Validate immediately on paste (text >3 chars) instead of requiring
blur first, so "Fund Platform Address" button activates without
needing to click away
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): make entire autocomplete row clickable in AddressInput
Previously only the address label was clickable — clicking the balance
text on the right side of the row did nothing. Now the click handler
uses the horizontal row response, so the entire row triggers selection.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): fix autocomplete click handling and format balances with 4dp
- Capture selectable_label click response instead of discarding it;
combine with row-level click for full-row clickability
- Format all DASH balances in dropdown with exactly 4 decimal places
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): use single interaction rect for full-row clickable autocomplete
Replace selectable_label + horizontal layout with allocate_exact_size
and manual painting — no child widgets steal clicks, the entire row
(address, balance, dead space) is clickable with hover feedback.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat(ui): add wallet address autocomplete to unshield screen
Populate AddressInput with wallet addresses via .with_wallets() so
users can select a destination from the dropdown instead of manually
entering addresses.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(error): handle asset lock and shielded insufficient funds errors with user-friendly messages
Add two new TaskError variants to replace raw SDK errors with actionable,
jargon-free messages:
- AssetLockOutPointInsufficientBalance: shown when an asset lock outpoint
has been partially consumed and lacks credits for the operation. Extracts
credits_left/credits_required and displays DASH amounts.
- ShieldedAddressInsufficientFunds: shown when a shielded broadcast fails
because the address balance is too low. Detected in shielded_broadcast_error()
before falling through to ShieldedBroadcastFailed.
Both follow the IdentityInsufficientBalance pattern with format_credits_as_dash.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): use AmountInput component for all amount inputs in wallet screens
Replace raw text_edit_singleline + custom parse_amount_*() methods with
the AmountInput component in 4 shielded screens (unshield, shield,
shield-from-asset-lock, shielded-send). This fixes a bug where entering
"1" would send 1 credit instead of 1 DASH because the raw input had
ambiguous parsing. Also adds "Amount (DASH):" label to SendScreen's
AmountInput and removes its redundant manual label.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* docs: add UI components reference and teach CLAUDE.md to use it
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(ui): redesign wallet screen information architecture
Replace the old Balances|Shielded top-level tabs and account dropdown
with a unified layout that better matches how users think about their
wallet:
- Balance section with collapsible breakdown (Core/Platform split)
- Dev Tools expandable button replaces scattered dev-mode controls
- Transaction History shown as collapsible section (visible in all modes)
- Accounts & Addresses use tabs instead of a dropdown selector
- Shielded view moved from top-level tab to an account tab
- Asset Locks restricted to the Dash Core tab only
- "Main Account" renamed to "Dash Core" throughout
- Fee column added to transaction table (dev mode only)
- "Get Test Dash" button opens testnet faucet in dev tools
Tab visibility follows progressive disclosure: default mode shows only
Dash Core, Platform, and Shielded tabs. Developer mode reveals all
account types that have addresses.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): use AddressInput component in Mine dialog for core address selection
Replace the manual ComboBox address selector in the Mine Blocks dialog
with the unified AddressInput component, configured for Core-only
addresses with selection-only mode from the current wallet.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): trigger shielded balance refresh after all shielding operations
After Shield, Shield from Core, Send Private, Unshield, and Send Dash
(with shielded source) complete successfully, automatically dispatch a
SyncNotes task so the shielded tab shows updated balances without
manual refresh.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* chore: demote cookie auth fallback log to trace level
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): prevent transaction list showing wrong wallet data
Add defensive checks in render_transactions_section to prevent
cross-wallet transaction leakage:
1. Verify the selected wallet Arc matches the canonical one in
app_context.wallets (guards against stale references).
2. Filter displayed transactions to only those with at least one
output matching the wallet's known addresses (prevents showing
transactions from other wallets that leaked in via a
non-wallet-scoped RPC endpoint).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): auto-show zero-balance addresses when wallet is empty
When all addresses have zero balance and there are fewer than 5
addresses total, bypass the zero-balance filter so new/empty wallets
show their addresses instead of a blank list. The checkbox remains
functional for wallets with balances.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): address PR review — shielded balance, dev tools layout
1. Include shielded balance in the collapsible balance breakdown
2. Add shielded balance to the total wallet balance everywhere
3. Right-align the Dev Tools button in the action row
4. Always show downward arrow on Dev Tools button
5. Dev Tools opens as a vertical dropdown popup instead of
a horizontal inline row
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): rename transaction heading, add explorer link, merge zk-fixes
- Rename "Transaction History" to "Dash Core Transaction History"
- Add "View" button on transactions for Mainnet/Testnet (opens Insight explorer)
- Merge zk-fixes (mine dialog, shielded refresh, wallet bug fixes, log level)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): trigger shielded sync on wallet refresh and wallet switch
The wallet Refresh button now dispatches a SyncNotes task alongside the
core wallet refresh when the shielded wallet has been initialized.
Switching HD wallets also triggers a full refresh (core + shielded) on
the next frame for unlocked wallets.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(log): add diagnostic logging for shielded transfer operations
Add tracing::info! and tracing::debug! logs throughout the shielded
transfer pipeline to enable post-mortem diagnosis when balances don't
update after broadcast. Key additions:
- bundle.rs: log amount, input notes, change before building each
shielded operation (transfer, shield, unshield, withdrawal,
shield-from-asset-lock); log broadcast success with note that
balance updates after next block
- sync.rs: warn when next_start_index is 0 (full rescan); log
post-sync balance with unspent note count
- context/shielded.rs: log note spend marking with before/after
unspent counts in with_anchor_retry
- shielded_send_screen.rs: log task result variant received and
post-transfer sync completion
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): improve Dev Tools dropdown layout and refresh mode cycling
Right-align popup content to match the Dev Tools button position.
Replace ComboBox with a cycle-on-click button to avoid nested popup
conflicts in egui, where the inner ComboBox dropdown would close the
parent popup.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): fix shielded balance conversion, add tab balances, reorder sync status
- Fix shielded balance inflating total: divide credits by CREDITS_PER_DUFF
before summing with duffs-denominated balances (was showing 5000 DASH
instead of 5 DASH)
- Show balances in account tab labels: Dash Core (0.9980) | Platform (0.1000)
| Shielded (5.0000), with (empty) for zero-balance tabs
- Move Sync Status section inside detail panel between balance breakdown
and action buttons for better visual flow
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(ui): collapsible sections, move tx history to Dash Core tab, restore account types
- Wrap Addresses, Transaction History, Asset Locks, and Shielded Notes
in collapsible headers (default open, tx history default closed)
- Move Dash Core Transaction History from top-level into the Dash Core
tab, between Addresses and Asset Locks
- In developer mode, show ALL account category tabs (Bip44, Platform,
Bip32, CoinJoin, Identity Registration/System/Top-up/Invitation,
Provider) even when no addresses exist for that type
- Add TODO for shielded tab layout redesign
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): improve shielded transfer UX messaging for balance update delays
After a shielded transfer, the sender's change notes and the recipient's
new notes won't appear until the next block is mined and synced. Users
see their balance temporarily drop to 0 and think the transfer failed.
Add informational messages on all three shielded transfer screens
(ShieldedSendScreen, UnshieldCreditsScreen, WalletSendScreen) explaining
that balances will update after the next block is confirmed. For
shielded-to-shielded transfers, also note that the recipient needs to
sync after the next block.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(ui): consolidate dev-mode accounts into System tab, limit balance decimals
Replace all individual dev-mode-only account type tabs (Identity Registration,
Identity System, Identity Top-up, Identity Invitation, CoinJoin, Provider,
Legacy BIP32) with a single "System" tab. Inside the System tab, each account
type appears as a collapsible section (collapsed by default) showing address
count and balance.
Tab order: Dash Core | Platform | Shielded | System (dev-mode only, always last)
Additional changes:
- Limit tab balance display to max 4 decimal places (was 8)
- Rename "Dev Tools" button to "Tools"
- Simplify refresh mode button: single button with "Refresh mode: X" text,
no separate label or arrow indicator
- Remove "Accounts & Addresses" section heading
- Style active tab with underline for visual distinction
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(shielded): scope commitment tree per wallet for multi-wallet correctness
Each wallet's ClientPersistentCommitmentTree now uses its own dedicated
SQLite file under <data_dir>/shielded/<hex>.db instead of sharing the
main database's global commitment_tree_* tables. This prevents wallets
from stepping on each other's Merkle tree state, which caused invalid
witnesses and silently rejected transactions.
Changes:
- SHLD-001: Per-wallet commitment tree DB files via ClientPersistentCommitmentTree::open()
- SHLD-002: clear_commitment_tree_for_wallet() scoped to a single wallet
- SHLD-007: Safety-net resync guard now logs wallet seed hash before clearing
- Migration v34: drops orphaned global commitment_tree_* tables
- clear_network_data() deletes per-wallet tree files for the target network
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(shielded): prevent permanent state leak on sync_notes failure in with_anchor_retry
When sync_notes() failed after an anchor mismatch, the early `?` return
skipped re-inserting the shielded state back into the HashMap, permanently
orphaning it until app restart. Now the sync result is captured without
early return, ensuring the state is always re-inserted regardless of
success or failure.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): apply password change for current session even when config save fails
The in-memory config update and SDK reinit only ran on the save-success
path, silently discarding the password change on save failure despite
the banner promising session-level application. Now the in-memory update
and reinit run unconditionally; only the banner text varies across the
four (save x reinit) outcome combinations.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(error): use dedicated TaskError variants for non-build wallet errors
reload_utxos() and recalculate_affected_address_balances() errors were
routed through shielded_build_error() which pattern-matches for build-
specific patterns like "AnchorMismatch". These are wallet operations,
not shielded builds. Added WalletUtxoReloadFailed and
WalletBalanceRecalculationFailed variants with appropriate user-facing
messages.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(error): preserve error chain in CoreRpcConnectionFailed
chain_lock_rpc_error() took the RPC error by reference and created
CoreRpcConnectionFailed with source: None, dropping all diagnostic
info. Added a detail: Option<String> field to the variant to carry
the formatted error. Boxed the source field to keep the enum size
under the clippy threshold. Updated all constructors.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(ui): two-column wallet header layout, rename Tools to Advanced
Split the wallet detail panel header into left (name, total balance,
action buttons) and right (collapsible balance breakdown, sync status)
columns. Renamed the "Tools" dropdown to "Advanced" and moved it into
the action buttons row instead of right-aligning it separately.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(test): rename test_v33_migration_fresh_install to match DB version 34
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* revert(shielded): use shared main DB for commitment tree instead of per-wallet files
Reverts the per-wallet SQLite file approach (63ce0e18). Multiple wallets
share the same commitment_tree_* tables in the main database. This is
accepted behavior until the SDK adds proper wallet-scoping support
(dashpay/grovedb#653).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(shielded): fix stale clear_commitment_tree_for_wallet reference after merge
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(shielded): harden anchor retry, commitment tree clearing, and state guard
SEC-002: Verify shielded balance after anchor retry sync before retrying
the operation — prevents doomed retries with zero balance.
SEC-003: Handle missing commitment tree tables in clear_commitment_tree_tables
(grovedb creates them lazily, so DELETEs fail on fresh installs).
Fix clear_network_data to log-and-continue when commitment tree clearing
fails — these tables are optional and shouldn't block network data reset.
Add SEC-001 INTENTIONAL annotation for panic safety in with_anchor_retry.
Document shield_from_asset_lock_task: anchor retry is not applicable since
it only reads the payment address and doesn't use the commitment tree.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(core): sanitize RPC errors and simplify CoreRpcConnectionFailed
SEC-006: Store user-friendly message instead of raw RPC error string
in network status when chain lock query fails with a non-auth,
non-connection error.
CODE-002: Remove redundant detail field from CoreRpcConnectionFailed --
format diagnostic info into the url field instead, keeping source for
the error chain.
Add SEC-005 INTENTIONAL annotation for RPC errors in status tooltip.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): config permissions, address detection, and per-frame caching
SEC-007: Set config file permissions to 0600 on Unix after save (contains
RPC credentials).
CODE-003: Deduplicate Amount::dash_from_duffs by delegating to
dash_from_credits.
CODE-006: Remove unused network parameter from AddressKind::detect --
detection is format-based, network validation happens separately.
CODE-004/CODE-005: Cache filtered transaction indices per wallet to
avoid rebuilding HashSet and filtering on every frame. Invalidated
on wallet switch and refresh.
CODE-008: Reset AddressInput widgets on network switch so they pick up
the new network for validation. Add invalidate_address_input methods
to SendScreen, UnshieldCreditsScreen, and WalletsBalancesScreen.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(logging): correct log levels per coding best practices
- debug! -> trace!: "state transition built, broadcasting" messages in
bundle.rs (5 occurrences) — primary-path step-by-step progress
- trace! -> debug!: cookie auth fallback in core/mod.rs and context/mod.rs
(2 occurrences) — secondary/fallback execution path, not primary flow
- info! -> debug!: anchor mismatch retry in context/shielded.rs — error
handling branch, not a business event
- info! -> debug!: "marked N note(s) spent" in context/shielded.rs —
internal bookkeeping, not a user-visible business event
- info! -> debug!: post-transfer sync complete in shielded_send_screen.rs —
internal plumbing at screen level
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* chore: minor ui fixes
* feat(ui): replace Advanced dropdown with inline right-aligned buttons
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): use allocate_ui_with_layout to right-align dev buttons
The right_to_left layout inside with_layout only got the remaining
space after left buttons, centering the dev buttons instead of
pushing them to the right edge. Using allocate_ui_with_layout with
the full remaining width forces the right-to-left block to span
the entire remaining area.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): eagerly initialize shielded balance on wallet screen open
The shielded tab balance only recalculated when the user clicked the
Shielded tab because ShieldedTabView was lazily initialized on tab
selection. This meant the wallet list and tab header showed zero balance
until the user manually visited the tab.
Two-layer fix:
- Backend: call initialize_shielded_wallet() in handle_wallet_unlocked()
so persisted shielded balance is loaded into shielded_states at wallet
load time, making it available to all UI screens immediately.
- UI: eagerly create ShieldedTabView on wallet screen construction and
wallet switch, and tick() it every frame so the init/sync chain
(InitializeShieldedWallet -> SyncNotes -> CheckNullifiers) runs even
when the Shielded tab is not the active tab.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: remove stale balance_breakdown_expanded field from cherry-pick
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): move action buttons to full-width row below header columns
The button bar was inside the left column (55% width), so the
right-aligned dev buttons ended up in the middle of the screen.
Move render_action_buttons() below the two-column header so it
spans the full available width.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor(ui): change sync status to bullet-point layout
Replace the wide horizontal Platform line (pipe-separated) with
individual bullet-point items: Core, Addresses, Notes, Nullifiers
each on their own line. Removes the Frame wrapper for a lighter
appearance.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(shielded): prevent double balance from redundant init + sync chain
When eager init (wallet_lifecycle) populates shielded_states and then
ShieldedTabView.tick() dispatches another InitializeShieldedWallet,
the idempotency check returns early but handle_result() chains
SyncNotes which can re-append notes already loaded from DB.
Fix: tick() now checks if shielded_states already contains the
wallet's state. If so, it adopts the cached balance and marks
itself initialized without dispatching a backend task, preventing
the redundant sync chain.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor(shielded): move initialization entirely to backend, remove UI init path
The shielded wallet had two initialization paths: the backend
(handle_wallet_unlocked -> initialize_shielded_wallet) and the UI
(ShieldedTabView::tick dispatching InitializeShieldedWallet). This
created redundant init attempts and blurred the responsibility boundary.
Now:
- handle_wallet_unlocked initializes the shielded state synchronously,
then queues SyncNotes -> CheckNullifiers as a background task
- ShieldedTabView reads its state from AppContext::shielded_states
via refresh_from_backend_state(), never dispatching init itself
- The "Initialize Shielded Wallet" button and WalletUnlockPopup are
removed from ShieldedTabView (wallet unlock already calls
handle_wallet_unlocked which handles everything)
- Resync Notes (developer mode) still dispatches InitializeShieldedWallet
as an explicit user action
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(shielded): deduplicate notes in sync to prevent double balance
When the commitment tree resets (empty/corrupt) but persisted notes
remain in the DB, initialize_shielded_wallet loads notes from DB
while last_synced_index falls to 0. The subsequent sync rescans
from position 0 and re-appends all decrypted notes — duplicating
the ones already in memory from DB, doubling the balance.
Fix: sync_notes() now checks if a note at the same position already
exists in shielded_state.notes before appending, preventing
duplicates regardless of the last_synced_index value.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat(ui): add shielded diversified address table
Replace the single-address display with prev/next buttons in the
Shielded tab with a collapsible address table showing all generated
diversified addresses. Each row shows the index, truncated bech32m
address (click or Copy button to copy full address), and status
(Default for index 0). The section is collapsed by default in normal
mode and expanded in developer mode.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address review findings — duplicate controls, perf, tab visibility
- Remove duplicate sync status/buttons block in shielded_tab.rs
- Fix BIP44 accounts with index > 0 being invisible (regression)
- Fix early return in render_account_tabs blocking Shielded tab for new wallets
- Fix double-tick of ShieldedTabView per frame when tab is active
- Fix system_tab_sections O(n*m) perf by precomputing address counts
- Track queue_shielded_sync via subtasks for graceful shutdown
- Use HashSet for O(1) duplicate note check in sync (was O(n^2))
- Fix balance_breakdown default_open to respect developer mode
- Fix is_system_category to delegate to is_visible_in_default_mode
- Fix doc comment for system_tab_sections sort order claim
- Narrow #[allow(dead_code)] to specific field on AccountSummary
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): replace mutex unwrap() with graceful error handling in shielded tab
Replace .lock().unwrap() calls on shielded_states with .lock().ok()
patterns throughout ShieldedTabView to prevent UI panics on poisoned
mutex. Shows fallback messages or skips updates gracefully.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(mcp): resolve async lifetime errors for Rust 2024 edition
Use spawn_blocking + block_on in dispatch_task to avoid Send bound
issues with platform SDK types (DataContract/Sdk references across
await points). Same pattern already used by AppState::handle_backend_task.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* chore: fix mcp dispatch
* doc: remove obsolete manual teting docs
* fix(ui): include zero-balance addresses in AddressInput wallet entries
Pull Core addresses from known_addresses (all derived) instead of
address_balances (only funded). Balance is looked up from
address_balances, defaulting to 0. Callers use with_balance_range(1..)
when they need funded-only addresses.
Fixes Mine dialog showing empty dropdown for fresh/zero-balance wallets.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): sort AddressInput wallet entries alphabetically
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* revert: remove redundant sort in extract_wallet_entries
filtered_entries() already sorts alphabetically by display_label.
The sort added in extract_wallet_entries was redundant.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): clarify shielded network validation logic
Replace double-negation with explicit mainnet-class comparison.
Documents that testnet/devnet/local share the same HRP ("tdash1z")
and cannot be distinguished at the address level.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): include send-all transactions in wallet history
Check both outputs and inputs when filtering transactions. Build an
OutPoint→Address lookup from the wallet's own transactions to resolve
input addresses. Send-all transactions (no change output) were
previously dropped because no output matched known_addresses.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): use is_ours flag for transaction filtering
Simplify transaction filter to use the is_ours flag instead of
address matching. Fix SPV path to set is_ours=true for all wallet
transactions (upstream only sets it for sends). SPV history is
per-wallet, so all entries are ours by definition.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* test(e2e): verify is_ours flag for SPV send and receive transactions
Sends funds between two wallets via SPV and asserts that both the
sender (negative net_amount) and receiver (positive net_amount) have
is_ours=true. Validates the fix to the SPV reconcile path that
overrides the upstream library's send-only is_ours logic.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(core): clear RPC error state on successful chain lock fetch
When get_best_chain_lock() succeeds on the active network, explicitly
clear any lingering RPC error in ConnectionStatus so the connection
indicator recovers after a transient outage.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(model): move is_platform_address_string from UI helpers to model layer
AddressKind::detect() in the model layer depended on crate::ui::helpers
for platform address detection — wrong layering. Move the function to
src/model/address.rs and keep a re-export in helpers for existing callers.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): add actionable guidance to address validation error messages
Each validation error now tells the user what to do, not just what went
wrong: "check for typos", "check you are using the correct network",
"use all lowercase". Messages that already had guidance are unchanged.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): invalidate address inputs on all screens during context switch
TransferScreen, AddressBalanceScreen, and ShieldedSendScreen retained
stale address text from the previous network after a context switch.
Add invalidate_address_input() to each and call it from change_context().
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): clear password field on network switch when no config exists
When switching networks, if no config entry exists for the new network
(or the password is empty), the password input field now clears instead
of retaining the previous network's password.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): clear validated_destination in invalidate_address_input
WalletSendScreen and UnshieldCreditsScreen cleared the AddressInput
widget but left validated_destination with stale validation from the
previous network. Now both are cleared together.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): consume ShieldedNotesSynced to update shielded send screen state
After a shielded transfer, the post-transfer note sync result was only
logged but never used to update the UI. Now ShieldedNotesSynced updates
the balance display, clears the pending-update banner, and appends the
remaining balance to the success message.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* style: apply cargo +nightly fmt formatting
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(model): disambiguate Core vs Identity address detection by prefix
Core addresses on Dash start with X/Y (mainnet) or y/8/7 (testnet).
When the input starts with a known Core prefix, try Core first.
Otherwise try Identity first, since Identity IDs use the same Base58
alphabet but don't have Core address prefixes.
This fixes identity-only AddressInput mode rejecting valid Identity
IDs that happened to parse as Core addresses, and eliminates the
flaky test that had to skip Core-looking random identifiers.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(core): show actual RPC error on Networks page instead of generic message
The Networks page is a debugging tool — users need the real error text
to diagnose Dash Core issues. Replace sanitized "RPC error — check
Dash Core status" with the actual error from dashcore_rpc.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(db): resolve deadlock in clear_network_data
clear_network_data() held self.conn.lock() for the transaction, then
called clear_commitment_tree_tables() which tried to acquire the same
mutex — classic non-reentrant mutex deadlock. UI froze on "Delete Data".
Fix: scope the connection lock so it's released before the commitment
tree clearing acquires it again.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): replace mutex unwrap() with graceful .ok() in shielded_sync_task
The shielded_states mutex lock used .unwrap() which would panic on a
poisoned mutex. Replace with .ok()? to return None gracefully.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): replace RwLock unwrap() with graceful error handling in shielded tab
Replace wallets.read().unwrap() with .read().ok() and show an error
label instead of panicking on a poisoned RwLock.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(spv): add debug log when overriding is_ours for receive transactions
Log at debug level when SPV reports is_ours=false for a receive
transaction (net_amount >= 0) to detect upstream API behavior changes.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(shielded): log warning on note value divergence during position dedup
Use a HashMap<position, value> instead of HashSet<position> so we can
detect when a re-synced note at an existing position has a different
value — indicating potential data integrity issues.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* perf(model): avoid allocation in is_platform_address_string
Replace to_lowercase() with eq_ignore_ascii_case() for the HRP prefix
check. The HRP constants are ASCII-only so this is safe and avoids a
heap allocation on every call.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor(ui): remove unused label field and is_key_only from AccountSummary
The label field was constructed but never read outside the builder.
is_key_only() had no callers. Remove both instead of suppressing
dead_code warnings.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor(ui): avoid cloning full AccountTab enum in tab content match
Extract only the category data (clone of AccountCategory + index) before
the match instead of cloning the entire enum. This avoids unnecessary
allocation for the Shielded and System variants.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): clear validated_address on network switch in mine dialog
invalidate_address_inputs() cleared the AddressInput widget but left
the validated_address stale, which could reference an address from the
previous network.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor(model): move truncate_address to model layer and document ASCII precondition
Move truncate_address from address_input.rs and shielded_tab.rs into
src/model/address.rs as a parameterized public function. Both callers
now delegate to the shared implementation with their own prefix/suffix
lengths.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* docs(shielded): document why spawn_blocking trampoline is needed in queue_shielded_sync
The spawn_blocking(block_on(...)) pattern is required because async
methods on Arc<Self> produce non-'static futures due to a known Rust
compiler limitation (rust-lang/rust#100013). Document this to prevent
future removal attempts.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* style: apply nightly rustfmt formatting
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(wallet): bootstrap platform addresses on wallet creation
bootstrap_wallet_addresses only ran when known_addresses was empty,
but new_from_seed already derives one Core address. This meant
platform payment addresses were never bootstrapped for new wallets
— only populated later by network sync (which returns nothing for
fresh wallets with no on-chain activity).
Now checks for the presence of PlatformPayment addresses in
watched_addresses and runs bootstrap if missing. Bootstrap functions
are idempotent (add_address_if_not_exists).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(ui): show bootstrapped platform addresses in AddressInput
Platform addresses were only populated from platform_address_info
(network sync results). Fresh wallets with no on-chain activity had
empty platform_address_info, so no platform addresses appeared in
the Send screen autocomplete.
Now derives platform addresses from watched_addresses (which contains
all bootstrapped platform payment addresses), with balance looked up
from platform_address_info (defaulting to 0).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor(ui): pass account filter directly to render_address_table
Remove the `selected_account` field that was mutated as a side-effect
during rendering, causing the last-rendered collapsible section in the
System tab to overwrite previous sections' state. Now each render call
receives its `(AccountCategory, Option<u32>)` as a direct parameter.
Additionally, `system_tab_sections` now builds per-(category, index)
pair so multi-index accounts show correct per-index balance and address
counts instead of aggregating across all indices.
Fixes CODE-005, CODE-011, PROJ-004.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* perf(shielded): move wallet initialization to background thread
ZIP32 key derivation and DB reads in `initialize_shielded_wallet` ran
synchronously on the UI thread during `handle_wallet_unlocked()`.
Move the call to a `spawn_blocking` task tracked by `subtasks` so the
UI remains responsive. The shielded tab already handles the
"initializing" state gracefully via its `is_initialized` check.
Fixes CODE-010.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* test(spv): add unit tests for is_ours override logic
Extract the SPV is_ours override into a named function and add four
unit tests covering: outgoing already-ours, incoming not-ours (the
main override case), zero-amount edge case, and outgoing not-ours.
A comment explains why bloom filter false positive testing requires
mocking the SPV layer and is out of scope for unit tests.
Fixes PROJ-002.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* docs(user-stories): add stories for wallet tab redesign
Add WAL-021 through WAL-024 covering the tab-based navigation,
developer-mode System tab, collapsible transaction history, and
collapsible balance breakdown sections introduced in PR #791.
Fixes PROJ-007.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* style: apply nightly rustfmt formatting
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(ui): distinguish change addresses in AddressInput autocomplete
Core addresses now show "(change)" suffix for BIP44 change addresses
(m/44'/5'/0'/1/x). New with_exclude_change(true) builder method
filters them out — send inputs should not display change addresses
since users don't share them with others.
Change detection uses the existing DerivationPathHelpers::is_bip44_change
trait method from the wallet…
lklimek
added a commit
that referenced
this pull request
Mar 26, 2026
…798) * more work * more work * fix(spv): zero out stale per-address balances during reconciliation (#627) During SPV reconciliation, per_address_sum only contains addresses with current UTXOs. Addresses whose funds were fully spent never had their balance reset to zero, causing the address table to display stale non-zero balances even though UTXO count correctly showed 0. Now explicitly zeroes address_balances for any known address absent from the refreshed UTXO map before applying current sums. Closes #571 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fix: handle malformed YAML gracefully in load_testnet_nodes_from_yml (#613) * fix: handle malformed YAML gracefully in load_testnet_nodes_from_yml Replace .expect() with match expression to avoid panicking when .testnet_nodes.yml contains malformed YAML. Instead, logs the error with tracing::error and returns None, allowing the application to continue without crashing. Closes #557 Co-authored-by: lklimek <lklimek@users.noreply.github.com> * fix: propagate YAML parse errors to UI and remove unwrap calls - Change load_testnet_nodes_from_yml to return Result<Option<TestnetNodes>, String> so parse errors display in the UI error banner instead of only logging - Use explicit type annotation on serde_yaml_ng::from_str::<TestnetNodes> - Replace .unwrap() with .and_then() in fill_random_hpmn/fill_random_masternode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: lklimek <lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * chore: let Claude write manual test scenarios for PRs (#634) * chore: move doc/ contents into docs/ and update references Consolidate documentation under a single docs/ directory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: CLAUDE.md should write manual test scenarios for PRs --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * build(flatpak): use only-arches for dynamic protoc architecture selection (#603) * build(flatpak): use only-arches for dynamic protoc architecture selection Replace the fragile sed-based CI patching of the Flatpak manifest with Flatpak's native `only-arches` source selector. The protoc module now declares both x86_64 and aarch64 sources inline, and build-commands use a glob pattern (`protoc-*.zip`) so no per-arch fixup is needed. Changes: - flatpak manifest: add aarch64 protoc source with `only-arches`, use glob in unzip commands, remove stale CI-patching comment - CI workflow: remove `protoc-zip`/`protoc-sha256` matrix variables and the "Patch manifest for architecture" step https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG * fix(flatpak): use dest-filename for deterministic protoc extraction Use dest-filename to normalize both arch-specific protoc zips to a common name, avoiding glob expansion in build-commands. This ensures the unzip target is deterministic regardless of shell behavior in the Flatpak sandbox. https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG * build: update platform to b445b6f0 and remove rust-dashcore patches Update dashpay/platform dependency from d6f4eb9a to b445b6f0e0bd4863 (3.0.1 → 3.1.0-dev.1). Remove the [patch] section that pinned rust-dashcore crates to a separate rev, as the new platform commit resolves them correctly on its own. https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG --------- Co-authored-by: Claude <noreply@anthropic.com> * fix(ci): remove local path patches, use git deps for platform crates The [patch] section referenced ../platform local paths that don't exist in CI. Since dash-sdk already depends on the feat/zk branch, all transitive platform crates resolve correctly without patches. Also fixes a formatting issue in address_table.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): store wallet in DB before registering addresses The remove_utxos tests were failing because `register_test_address` inserts into `wallet_addresses` which has a foreign key constraint on `wallet(seed_hash)`. Added the missing `store_wallet` call so the parent row exists before inserting child address rows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(build): restore shielded module declaration removed during merge The `pub mod shielded;` declaration was accidentally removed from `src/model/wallet/mod.rs` during the v1.0-dev merge, causing build failures since the shielded.rs file still exists. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): restore store_wallet calls lost in merge (#663) * Initial plan * fix(test): restore store_wallet calls lost in merge 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> * Merge remote-tracking branch 'origin/v1.0-dev' into zk-extract/all-merged Resolve conflicts in Cargo.toml (keep feat/zk branch), Cargo.lock (regenerate with pinned platform rev 4d7b9be5), and backend_task/mod.rs (combine TaskError wrapping with ShieldedTask). Fix post-merge integration issues: - SPV manager: remove stale .await on subscribe methods, add command_receiver channel for updated DashSpvClient::run() API - send_screen: update SendStatus::WaitingForResult to unit variant - network_chooser_screen: handle new SyncState::Initializing variant Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: update platform dependency to 3.1-dev branch Migrate from feat/zk to 3.1-dev branch of dashpay/platform, adapting to breaking API changes in dash-spv, key-wallet, and dpp: - FeeLevel removed; use FeeRate::normal() directly - DashSpvClientInterface/Command removed; use DashSpvClient directly - SyncState::Initializing removed; replaced with WaitForEvents - NetworkExt trait inlined into Network impl - OrchardProver now requires wrapper struct around ProvingKey - OrchardAddress::from_raw_bytes now returns Result - Builder functions gain fee/platform_version params - NullifierSyncConfig API uses NullifierSyncCheckpoint - WalletManager.create_unsigned_payment_transaction removed; use TransactionBuilder directly - Work around Send lifetime issues with spawn_blocking Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: migrate shielded module from Result<T, String> to typed TaskError Replace all Result<T, String> error patterns in the shielded pool module with typed TaskError variants, aligning with the codebase-wide typed error migration (PR #739). New TaskError variants: ShieldedNoUnspentNotes, ShieldedInsufficientBalance, PlatformAddressNotFound, ShieldedMerkleWitnessUnavailable, ShieldedTransitionBuildFailed, ShieldedBroadcastFailed, ShieldedInvalidRecipientAddress, ShieldedAssetLockTimeout, ShieldedSyncFailed, ShieldedTreeUpdateFailed, ShieldedNullifierSyncFailed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): prevent settings password row from clipping right edge Reserve width for Save and Auto Update buttons so the password input doesn't consume all available space, pushing buttons off-screen. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(rpc): include host:port in connection-refused errors and always show details Add CoreRpcConnectionFailed variant to TaskError that includes the configured address in the user-facing message. Connection-refused errors are now detected via is_rpc_connection_error() and enriched with host:port at every RPC call site where the URL is known (AppContext::rpc_error_with_url helper). Details panel is now shown for all RPC-related errors regardless of developer mode, so users can always see the technical information they need to diagnose connectivity issues. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): save RPC password for active network instead of hardcoded Regtest The Network Chooser screen had three bugs related to RPC password handling: 1. Password input was initialized from Regtest config only, ignoring the current network selection. 2. Password UI was hidden for all networks except Regtest, even though Mainnet/Testnet/Devnet also use RPC mode. 3. Save logic was hardcoded to update Regtest config and triggered a SwitchNetwork(Regtest) action, which disconnected the active network's ZMQ listener unnecessarily. Now the password input shows for any network in RPC mode, reads/writes the correct network config, reinits the RPC client in-place without triggering a network switch, and reloads the stored password when the user switches network tabs. The "Auto Update" (dashmate) button remains Regtest-only since dashmate is only relevant for local networks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): ensure funding method dropdown fits all items without scrollbar Add explicit .height(200.0) to the funding method ComboBox in both top_up_identity_screen and add_new_identity_screen. The add_enabled_ui wrappers inflate item height via frame overhead, causing the popup to clip to a single row at the default height. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(error): show actionable message for insufficient identity balance IdentityInsufficientBalanceError from the SDK now maps to a dedicated TaskError::IdentityInsufficientBalance variant instead of falling through to "An unexpected error occurred." The user sees the available and required credit amounts along with a clear "top up your identity" action. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): clear stale error banners when saving RPC password Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use network-compatible comparison for platform address lookups Regtest and Testnet share the `tdash` bech32m prefix, but `PlatformAddress::from_bech32m_string()` always returns `Network::Testnet` for `tdash` addresses. The strict `!=` comparison in the fund-platform dialog rejected valid Regtest addresses with "Address network mismatch". - Make `networks_address_compatible()` `pub(crate)` in `model::wallet` so all modules can reuse the canonical check - Remove the duplicate private copy in `backend_task::core` and import from the single source - Replace `network != self.app_context.network` in `dialogs.rs` with `networks_address_compatible()` so Testnet/Devnet/Regtest addresses are accepted interchangeably Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(error): add actionable messages for shielded fee and pool-size errors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(error): add actionable message for shielded anchor mismatch Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(shielded): auto-resync notes and retry on anchor mismatch When unshield_credits, shielded_transfer, or shielded_withdrawal fails with ShieldedAnchorMismatch (stale commitment tree witnesses), automatically sync notes once to update the tree and retry the operation. Only one resync attempt is made — if the retry also fails, the error propagates as-is. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(shielded): ensure shielded tables exist and log DB errors during init Three changes to fix empty shielded balance after app restart: 1. Defensive table creation in initialize(): after migrations complete, ensure shielded_notes and shielded_wallet_meta tables exist even if the DB version was already past v29/v30 from a prior build. Both methods use CREATE TABLE IF NOT EXISTS, so this is safe. 2. Log DB errors during shielded init: change silent if-let-Ok pattern to match/Err with tracing::warn, so missing-table errors are visible instead of silently producing empty note lists. 3. Safety net resync: when the commitment tree has been synced but no unspent notes were loaded from DB, clear the tree and reset last_synced_index to 0 so the auto-sync rediscovers all notes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): consolidate migrations v28-v32 into v33 Resolves version numbering collision between zk and v1.0-dev branches: the zk branch used v28 for shielded tables while v1.0-dev used v28 for contacts. After merging, users migrating from either branch could end up with missing tables depending on which version their DB was at. v33 runs all sub-migrations idempotently in one step, ensuring all tables exist regardless of prior migration history. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): consolidate migrations v28-v32 into v33 Resolves version numbering collision between zk and v1.0-dev branches: the zk branch used v28 for shielded tables while v1.0-dev used v28 for contacts. After merging, users migrating from either branch could end up with missing tables depending on which version their DB was at. v33 runs all sub-migrations idempotently in one step, ensuring all tables exist regardless of prior migration history. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: pin platform dependency to zk-fixes revision Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): address PR review — fresh schema, error propagation, rename coverage - propagate SQL error in add_nullifier_sync_timestamp_column instead of swallowing it with unwrap_or(false) - add shielded_notes and shielded_wallet_meta to rename_network_dash_to_mainnet Note: Fix 1 (last_nullifier_sync_timestamp in CREATE TABLE) was already present on this branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): address PR review — fresh schema, error propagation, rename coverage - propagate SQL error in add_nullifier_sync_timestamp_column instead of swallowing it with unwrap_or(false) - add shielded_notes and shielded_wallet_meta to rename_network_dash_to_mainnet Note: Fix 1 (last_nullifier_sync_timestamp in CREATE TABLE) was already present on this branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): add foreign key constraints to shielded tables shielded_notes and shielded_wallet_meta both had wallet_seed_hash columns with no FK constraint. Added FOREIGN KEY (wallet_seed_hash) REFERENCES wallet(seed_hash) ON DELETE CASCADE to both, matching the pattern used by all other per-wallet tables in the codebase. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): add foreign key constraints to shielded tables shielded_notes and shielded_wallet_meta both had wallet_seed_hash columns with no FK constraint. Added FOREIGN KEY (wallet_seed_hash) REFERENCES wallet(seed_hash) ON DELETE CASCADE to both, matching the pattern used by all other per-wallet tables in the codebase. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): remove duplicate shielded methods after v1.0-dev merge On zk-fixes, shielded DB methods live in shielded.rs. The v1.0-dev backport inlined them in initialization.rs (no shielded.rs on that branch). Merging v1.0-dev back caused E0592 duplicate definitions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): remove duplicate wallet store in register_test_address register_test_address called db.store_wallet redundantly — callers already store the wallet before calling it, causing UNIQUE constraint violations when tests run in parallel on CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(shielded): address PR review — error propagation, retry helper, safety net - Propagate DB error from get_unspent_shielded_notes instead of swallowing - Extract with_anchor_retry() helper to deduplicate ~27 lines across shielded_transfer_task, unshield_credits_task, shielded_withdrawal_task - Fix safety net false positive: check all notes (spent + unspent) to distinguish "never had notes" from "all spent" - Propagate error from clear_commitment_tree_tables instead of discarding Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(error): address PR review — Amount formatting, Display completeness - Add Amount::dash_from_credits() constructor to model/amount.rs - Replace manual format_credits_as_dash() logic with Amount::dash_from_credits().to_string(), so credit amounts use Amount's Display (with DASH unit, trimmed trailing zeros) - Update ShieldedFeeExceedsBalance #[error] message: remove redundant "Dash" literal (Amount's Display already includes the "DASH" unit suffix) - Update format_credits_as_dash tests to match new output format ("1 DASH", "2.5 DASH", etc.) - Remove special-casing that showed with_details() for all RPC errors unconditionally: all required user-facing info is already in each variant's Display string; technical details are now shown only in developer mode (aligns with CLAUDE.md §error-messages rule 4) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(rpc): address PR review — context fallback, success banner, error source - Guard in-memory config update + reinit behind a check that the target network's AppContext actually exists; skip both when it doesn't so we don't accidentally overwrite mainnet config via the fallback path. - Show success banner only when reinit succeeds; show a warning when it fails so the user knows the connection may not reflect the new password. - Make CoreRpcConnectionFailed.source Option<dashcore_rpc::Error> so chain_lock_rpc_error can pass None instead of fabricating a fake ConnectionRefused error from a borrowed reference. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: simplify shielded helpers comment in initialization.rs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): address PR #789 review — doc comments, migration default - Split misplaced doc comment: `add_wallet_transaction_status_column` now has its own Migration 30 doc; `rename_network_dash_to_mainnet` gets its own Migration 29 doc. - Add inline comment explaining why the migration uses DEFAULT 2 (Confirmed) while fresh CREATE TABLE in wallet.rs uses DEFAULT 0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(db): remove duplicate shielded methods after v1.0-dev merge On zk-fixes, shielded DB methods live in shielded.rs. The v1.0-dev backport inlined them in initialization.rs (no shielded.rs on that branch). Merging v1.0-dev back caused E0592 duplicate definitions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: simplify shielded helpers comment in initialization.rs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore(db): document migration DEFAULT 2 vs fresh DEFAULT 0 Existing transactions predate status tracking and are assumed confirmed (DEFAULT 2). Fresh installs use DEFAULT 0 (Unconfirmed). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address review — zero-balance detection, status warning Add `spv_balance_known: bool` to `Wallet` to distinguish a synced zero-balance wallet from an unsynced one. `spv_confirmed_balance()` now returns `None` only before the first SPV report, and `Some(0)` after SPV confirms an empty wallet. Add `tracing::warn!` in `TransactionStatus::from_u8` when an unknown discriminant is encountered, avoiding silent data coercion in a financial context. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(error): address review — typed BIP32 source, migration error variant Replace WalletKeyDerivationFailed { detail: String } with a typed #[source] field (Box<dyn Error + Send + Sync>) so the error chain is preserved and Display/Debug separation is explicit. Update all callsites in wallet/mod.rs and identity/mod.rs accordingly. Replace the semantically wrong InvalidParameterName map_err in rename_network_dash_to_mainnet with a plain ? — execute() already returns rusqlite::Result so no conversion is needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test(db): add v33 consolidated migration regression tests Two test scenarios verify the v33 consolidated migration that merges sub-migrations v28-v32 (core_wallet_name, shielded tables, contacts, wallet transaction status, network rename): 1. Fresh install creates all v33 tables/columns directly via create_tables() 2. Upgrade from v27 runs the full migration path and produces identical schema Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): show warning when config save fails instead of success When config.save() fails, stop early with a warning banner instead of continuing to reinit and showing a misleading success message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(error): consolidate format_credits_as_dash and remove jargon from user messages - fee_estimation::format_credits_as_dash now delegates to Amount::dash_from_credits() instead of doing its own f64 math with fixed 8 decimals; output is now trimmed (e.g. "1 DASH" instead of "1.00000000 DASH") - Remove private format_credits_as_dash from error.rs; import the pub version from fee_estimation instead - IdentityInsufficientBalance: show amounts in DASH rather than raw credit integers - ShieldedAnchorMismatch: replace "out of sync" with plain-language retry guidance - ShieldedFeeExceedsBalance: remove "shielded balance" / "shield more credits" jargon - ShieldedInsufficientPoolNotes: remove count interpolations and ZK pool jargon - Update tests throughout to match new message content and format Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(test): remove duplicate wallet store in register_test_address register_test_address called db.store_wallet redundantly — callers already store the wallet before calling it, causing UNIQUE constraint violations when tests run in parallel on CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(core): log chain lock RPC errors that aren't auth/connection failures Silent swallowing of errors like "Unable to find any ChainLock" made it impossible to diagnose why the active network showed as Disconnected. Now warns via tracing when chain_lock_rpc_error returns None. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(ui): surface chain lock RPC errors in Networks tab Previously, non-auth/non-connection RPC errors (e.g. "Unable to find any ChainLock") were silently swallowed — the UI showed "Disconnected" with no explanation. Now the error message is carried through the ChainLocks result, stored in ConnectionStatus.rpc_last_error, and displayed as "Error: ..." in both developer and non-developer RPC status labels on the Networks tab. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(ui): unified AddressInput component with autocomplete (#787) * feat(ui): add unified AddressInput component with autocomplete Introduce a reusable AddressInput component that handles text input with real-time address type detection, autocomplete from wallet data, balance display, type filtering, and network-aware validation. Supports Core, Platform, Shielded, and Identity address kinds. New files: - src/model/address.rs: AddressKind enum and ValidatedAddress enum - src/ui/components/address_input.rs: full component with 33 unit tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(ui): integrate AddressInput component into send and unshield screens Replace inline address parsing and validation with the unified AddressInput component across 3 proof-of-concept sites: - UnshieldCreditsScreen: full migration, removes local Destination enum and parse_destination(), gains autocomplete and type-restricted input - WalletSendScreen simple mode: full migration, removes AddressType enum, replaces detect_address_type/is_shielded_address with AddressKind-based detection, eliminates double-parsing in send handlers - WalletSendScreen advanced mode: minimal migration, updates type detection to use AddressKind, keeps CoreAddressInput/PlatformAddressInput structs unchanged Net reduction of ~47 lines per screen. All send flows preserved. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(address-input): fix misleading test and add missing coverage The existing `disabled_type_rejected_with_correct_error` test was testing empty input (which returns no error) rather than actually verifying type restriction. Fixed the test and added coverage for: - Selection-only mode rejection of manual input - Identity validation with valid/invalid identifiers - Truncate boundary at exactly 16/17 characters - Empty input in selection-only and restricted-type modes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): address QA findings for AddressInput component QA-001: Extract duplicated address detection logic from send_screen.rs into AddressKind::detect() on the model type. Both send_screen and address_input now delegate to the single canonical implementation. QA-002: Fix autocomplete "...and N more" count using unfiltered total. filtered_entries() now returns the pre-truncation match count so the overflow label shows the correct number of remaining matches. QA-003: Add minimum length check (>= 60 chars) to shielded address validation. Previously any string with the correct prefix was accepted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): address bot review findings for AddressInput component - Validate shielded addresses via OrchardAddress::from_bech32m_string() instead of prefix+length check only - Use char-aware truncation in truncate_address() to prevent panics on multi-byte UTF-8 input (DPNS labels, emoji) - Reset AddressInput when source selection changes in send screen, and configure allowed destination kinds per source type - Store bech32m string in ValidatedAddress::Platform variant so to_address_string() returns canonical encoding instead of debug hex Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): address remaining review findings for AddressInput component - Fix manual entry not propagating validated address on blur (HIGH) - Fix selected_from_autocomplete causing repeated change signals - Remove protocol jargon from unshield screen messages - Reject mixed-case bech32m platform addresses per BIP-350 - Use per-instance ComboBox ID to prevent state collision - Clear cached_detection after autocomplete selection - Fix doc comments and reuse filtered_entries() computation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(ui): support multi-wallet autocomplete in AddressInput Replace with_wallet/set_wallet (single wallet) with with_wallets/set_wallets (slice of wallets). When multiple wallets are loaded, each autocomplete entry is prefixed with the wallet alias so the user can tell which wallet owns the address. send_screen.rs now passes all loaded wallets to AddressInput instead of only the selected one. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ui): restore missing "Show zero-balance addresses" checkbox The checkbox was accidentally dropped during a branch merge. Restored the horizontal layout with heading on the left and checkbox right-aligned, matching the v1.0-dev implementation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): improve AddressInput autocomplete UX - Fix click selection: keep popup rendered when text field loses focus so the click handler fires (was gated on has_focus only) - Show dropdown on focus with any input length (removed 3-char minimum) - Add type suffix (Core), (Platform), (Identity), (Shielded) to dropdown entries when multiple address types are enabled - Validate immediately on paste (text >3 chars) instead of requiring blur first, so "Fund Platform Address" button activates without needing to click away Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): make entire autocomplete row clickable in AddressInput Previously only the address label was clickable — clicking the balance text on the right side of the row did nothing. Now the click handler uses the horizontal row response, so the entire row triggers selection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): fix autocomplete click handling and format balances with 4dp - Capture selectable_label click response instead of discarding it; combine with row-level click for full-row clickability - Format all DASH balances in dropdown with exactly 4 decimal places Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): use single interaction rect for full-row clickable autocomplete Replace selectable_label + horizontal layout with allocate_exact_size and manual painting — no child widgets steal clicks, the entire row (address, balance, dead space) is clickable with hover feedback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(ui): add wallet address autocomplete to unshield screen Populate AddressInput with wallet addresses via .with_wallets() so users can select a destination from the dropdown instead of manually entering addresses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(error): handle asset lock and shielded insufficient funds errors with user-friendly messages Add two new TaskError variants to replace raw SDK errors with actionable, jargon-free messages: - AssetLockOutPointInsufficientBalance: shown when an asset lock outpoint has been partially consumed and lacks credits for the operation. Extracts credits_left/credits_required and displays DASH amounts. - ShieldedAddressInsufficientFunds: shown when a shielded broadcast fails because the address balance is too low. Detected in shielded_broadcast_error() before falling through to ShieldedBroadcastFailed. Both follow the IdentityInsufficientBalance pattern with format_credits_as_dash. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): use AmountInput component for all amount inputs in wallet screens Replace raw text_edit_singleline + custom parse_amount_*() methods with the AmountInput component in 4 shielded screens (unshield, shield, shield-from-asset-lock, shielded-send). This fixes a bug where entering "1" would send 1 credit instead of 1 DASH because the raw input had ambiguous parsing. Also adds "Amount (DASH):" label to SendScreen's AmountInput and removes its redundant manual label. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add UI components reference and teach CLAUDE.md to use it Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(ui): redesign wallet screen information architecture Replace the old Balances|Shielded top-level tabs and account dropdown with a unified layout that better matches how users think about their wallet: - Balance section with collapsible breakdown (Core/Platform split) - Dev Tools expandable button replaces scattered dev-mode controls - Transaction History shown as collapsible section (visible in all modes) - Accounts & Addresses use tabs instead of a dropdown selector - Shielded view moved from top-level tab to an account tab - Asset Locks restricted to the Dash Core tab only - "Main Account" renamed to "Dash Core" throughout - Fee column added to transaction table (dev mode only) - "Get Test Dash" button opens testnet faucet in dev tools Tab visibility follows progressive disclosure: default mode shows only Dash Core, Platform, and Shielded tabs. Developer mode reveals all account types that have addresses. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): use AddressInput component in Mine dialog for core address selection Replace the manual ComboBox address selector in the Mine Blocks dialog with the unified AddressInput component, configured for Core-only addresses with selection-only mode from the current wallet. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): trigger shielded balance refresh after all shielding operations After Shield, Shield from Core, Send Private, Unshield, and Send Dash (with shielded source) complete successfully, automatically dispatch a SyncNotes task so the shielded tab shows updated balances without manual refresh. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: demote cookie auth fallback log to trace level Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): prevent transaction list showing wrong wallet data Add defensive checks in render_transactions_section to prevent cross-wallet transaction leakage: 1. Verify the selected wallet Arc matches the canonical one in app_context.wallets (guards against stale references). 2. Filter displayed transactions to only those with at least one output matching the wallet's known addresses (prevents showing transactions from other wallets that leaked in via a non-wallet-scoped RPC endpoint). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): auto-show zero-balance addresses when wallet is empty When all addresses have zero balance and there are fewer than 5 addresses total, bypass the zero-balance filter so new/empty wallets show their addresses instead of a blank list. The checkbox remains functional for wallets with balances. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): address PR review — shielded balance, dev tools layout 1. Include shielded balance in the collapsible balance breakdown 2. Add shielded balance to the total wallet balance everywhere 3. Right-align the Dev Tools button in the action row 4. Always show downward arrow on Dev Tools button 5. Dev Tools opens as a vertical dropdown popup instead of a horizontal inline row Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): rename transaction heading, add explorer link, merge zk-fixes - Rename "Transaction History" to "Dash Core Transaction History" - Add "View" button on transactions for Mainnet/Testnet (opens Insight explorer) - Merge zk-fixes (mine dialog, shielded refresh, wallet bug fixes, log level) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): trigger shielded sync on wallet refresh and wallet switch The wallet Refresh button now dispatches a SyncNotes task alongside the core wallet refresh when the shielded wallet has been initialized. Switching HD wallets also triggers a full refresh (core + shielded) on the next frame for unlocked wallets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(log): add diagnostic logging for shielded transfer operations Add tracing::info! and tracing::debug! logs throughout the shielded transfer pipeline to enable post-mortem diagnosis when balances don't update after broadcast. Key additions: - bundle.rs: log amount, input notes, change before building each shielded operation (transfer, shield, unshield, withdrawal, shield-from-asset-lock); log broadcast success with note that balance updates after next block - sync.rs: warn when next_start_index is 0 (full rescan); log post-sync balance with unspent note count - context/shielded.rs: log note spend marking with before/after unspent counts in with_anchor_retry - shielded_send_screen.rs: log task result variant received and post-transfer sync completion Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): improve Dev Tools dropdown layout and refresh mode cycling Right-align popup content to match the Dev Tools button position. Replace ComboBox with a cycle-on-click button to avoid nested popup conflicts in egui, where the inner ComboBox dropdown would close the parent popup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): fix shielded balance conversion, add tab balances, reorder sync status - Fix shielded balance inflating total: divide credits by CREDITS_PER_DUFF before summing with duffs-denominated balances (was showing 5000 DASH instead of 5 DASH) - Show balances in account tab labels: Dash Core (0.9980) | Platform (0.1000) | Shielded (5.0000), with (empty) for zero-balance tabs - Move Sync Status section inside detail panel between balance breakdown and action buttons for better visual flow Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(ui): collapsible sections, move tx history to Dash Core tab, restore account types - Wrap Addresses, Transaction History, Asset Locks, and Shielded Notes in collapsible headers (default open, tx history default closed) - Move Dash Core Transaction History from top-level into the Dash Core tab, between Addresses and Asset Locks - In developer mode, show ALL account category tabs (Bip44, Platform, Bip32, CoinJoin, Identity Registration/System/Top-up/Invitation, Provider) even when no addresses exist for that type - Add TODO for shielded tab layout redesign Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): improve shielded transfer UX messaging for balance update delays After a shielded transfer, the sender's change notes and the recipient's new notes won't appear until the next block is mined and synced. Users see their balance temporarily drop to 0 and think the transfer failed. Add informational messages on all three shielded transfer screens (ShieldedSendScreen, UnshieldCreditsScreen, WalletSendScreen) explaining that balances will update after the next block is confirmed. For shielded-to-shielded transfers, also note that the recipient needs to sync after the next block. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(ui): consolidate dev-mode accounts into System tab, limit balance decimals Replace all individual dev-mode-only account type tabs (Identity Registration, Identity System, Identity Top-up, Identity Invitation, CoinJoin, Provider, Legacy BIP32) with a single "System" tab. Inside the System tab, each account type appears as a collapsible section (collapsed by default) showing address count and balance. Tab order: Dash Core | Platform | Shielded | System (dev-mode only, always last) Additional changes: - Limit tab balance display to max 4 decimal places (was 8) - Rename "Dev Tools" button to "Tools" - Simplify refresh mode button: single button with "Refresh mode: X" text, no separate label or arrow indicator - Remove "Accounts & Addresses" section heading - Style active tab with underline for visual distinction Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(shielded): scope commitment tree per wallet for multi-wallet correctness Each wallet's ClientPersistentCommitmentTree now uses its own dedicated SQLite file under <data_dir>/shielded/<hex>.db instead of sharing the main database's global commitment_tree_* tables. This prevents wallets from stepping on each other's Merkle tree state, which caused invalid witnesses and silently rejected transactions. Changes: - SHLD-001: Per-wallet commitment tree DB files via ClientPersistentCommitmentTree::open() - SHLD-002: clear_commitment_tree_for_wallet() scoped to a single wallet - SHLD-007: Safety-net resync guard now logs wallet seed hash before clearing - Migration v34: drops orphaned global commitment_tree_* tables - clear_network_data() deletes per-wallet tree files for the target network Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(shielded): prevent permanent state leak on sync_notes failure in with_anchor_retry When sync_notes() failed after an anchor mismatch, the early `?` return skipped re-inserting the shielded state back into the HashMap, permanently orphaning it until app restart. Now the sync result is captured without early return, ensuring the state is always re-inserted regardless of success or failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): apply password change for current session even when config save fails The in-memory config update and SDK reinit only ran on the save-success path, silently discarding the password change on save failure despite the banner promising session-level application. Now the in-memory update and reinit run unconditionally; only the banner text varies across the four (save x reinit) outcome combinations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(error): use dedicated TaskError variants for non-build wallet errors reload_utxos() and recalculate_affected_address_balances() errors were routed through shielded_build_error() which pattern-matches for build- specific patterns like "AnchorMismatch". These are wallet operations, not shielded builds. Added WalletUtxoReloadFailed and WalletBalanceRecalculationFailed variants with appropriate user-facing messages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(error): preserve error chain in CoreRpcConnectionFailed chain_lock_rpc_error() took the RPC error by reference and created CoreRpcConnectionFailed with source: None, dropping all diagnostic info. Added a detail: Option<String> field to the variant to carry the formatted error. Boxed the source field to keep the enum size under the clippy threshold. Updated all constructors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(ui): two-column wallet header layout, rename Tools to Advanced Split the wallet detail panel header into left (name, total balance, action buttons) and right (collapsible balance breakdown, sync status) columns. Renamed the "Tools" dropdown to "Advanced" and moved it into the action buttons row instead of right-aligning it separately. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(test): rename test_v33_migration_fresh_install to match DB version 34 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * revert(shielded): use shared main DB for commitment tree instead of per-wallet files Reverts the per-wallet SQLite file approach (63ce0e18). Multiple wallets share the same commitment_tree_* tables in the main database. This is accepted behavior until the SDK adds proper wallet-scoping support (dashpay/grovedb#653). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(shielded): fix stale clear_commitment_tree_for_wallet reference after merge Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(shielded): harden anchor retry, commitment tree clearing, and state guard SEC-002: Verify shielded balance after anchor retry sync before retrying the operation — prevents doomed retries with zero balance. SEC-003: Handle missing commitment tree tables in clear_commitment_tree_tables (grovedb creates them lazily, so DELETEs fail on fresh installs). Fix clear_network_data to log-and-continue when commitment tree clearing fails — these tables are optional and shouldn't block network data reset. Add SEC-001 INTENTIONAL annotation for panic safety in with_anchor_retry. Document shield_from_asset_lock_task: anchor retry is not applicable since it only reads the payment address and doesn't use the commitment tree. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(core): sanitize RPC errors and simplify CoreRpcConnectionFailed SEC-006: Store user-friendly message instead of raw RPC error string in network status when chain lock query fails with a non-auth, non-connection error. CODE-002: Remove redundant detail field from CoreRpcConnectionFailed -- format diagnostic info into the url field instead, keeping source for the error chain. Add SEC-005 INTENTIONAL annotation for RPC errors in status tooltip. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): config permissions, address detection, and per-frame caching SEC-007: Set config file permissions to 0600 on Unix after save (contains RPC credentials). CODE-003: Deduplicate Amount::dash_from_duffs by delegating to dash_from_credits. CODE-006: Remove unused network parameter from AddressKind::detect -- detection is format-based, network validation happens separately. CODE-004/CODE-005: Cache filtered transaction indices per wallet to avoid rebuilding HashSet and filtering on every frame. Invalidated on wallet switch and refresh. CODE-008: Reset AddressInput widgets on network switch so they pick up the new network for validation. Add invalidate_address_input methods to SendScreen, UnshieldCreditsScreen, and WalletsBalancesScreen. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(logging): correct log levels per coding best practices - debug! -> trace!: "state transition built, broadcasting" messages in bundle.rs (5 occurrences) — primary-path step-by-step progress - trace! -> debug!: cookie auth fallback in core/mod.rs and context/mod.rs (2 occurrences) — secondary/fallback execution path, not primary flow - info! -> debug!: anchor mismatch retry in context/shielded.rs — error handling branch, not a business event - info! -> debug!: "marked N note(s) spent" in context/shielded.rs — internal bookkeeping, not a user-visible business event - info! -> debug!: post-transfer sync complete in shielded_send_screen.rs — internal plumbing at screen level Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: minor ui fixes * feat(ui): replace Advanced dropdown with inline right-aligned buttons Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): use allocate_ui_with_layout to right-align dev buttons The right_to_left layout inside with_layout only got the remaining space after left buttons, centering the dev buttons instead of pushing them to the right edge. Using allocate_ui_with_layout with the full remaining width forces the right-to-left block to span the entire remaining area. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): eagerly initialize shielded balance on wallet screen open The shielded tab balance only recalculated when the user clicked the Shielded tab because ShieldedTabView was lazily initialized on tab selection. This meant the wallet list and tab header showed zero balance until the user manually visited the tab. Two-layer fix: - Backend: call initialize_shielded_wallet() in handle_wallet_unlocked() so persisted shielded balance is loaded into shielded_states at wallet load time, making it available to all UI screens immediately. - UI: eagerly create ShieldedTabView on wallet screen construction and wallet switch, and tick() it every frame so the init/sync chain (InitializeShieldedWallet -> SyncNotes -> CheckNullifiers) runs even when the Shielded tab is not the active tab. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: remove stale balance_breakdown_expanded field from cherry-pick Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): move action buttons to full-width row below header columns The button bar was inside the left column (55% width), so the right-aligned dev buttons ended up in the middle of the screen. Move render_action_buttons() below the two-column header so it spans the full available width. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): change sync status to bullet-point layout Replace the wide horizontal Platform line (pipe-separated) with individual bullet-point items: Core, Addresses, Notes, Nullifiers each on their own line. Removes the Frame wrapper for a lighter appearance. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(shielded): prevent double balance from redundant init + sync chain When eager init (wallet_lifecycle) populates shielded_states and then ShieldedTabView.tick() dispatches another InitializeShieldedWallet, the idempotency check returns early but handle_result() chains SyncNotes which can re-append notes already loaded from DB. Fix: tick() now checks if shielded_states already contains the wallet's state. If so, it adopts the cached balance and marks itself initialized without dispatching a backend task, preventing the redundant sync chain. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(shielded): move initialization entirely to backend, remove UI init path The shielded wallet had two initialization paths: the backend (handle_wallet_unlocked -> initialize_shielded_wallet) and the UI (ShieldedTabView::tick dispatching InitializeShieldedWallet). This created redundant init attempts and blurred the responsibility boundary. Now: - handle_wallet_unlocked initializes the shielded state synchronously, then queues SyncNotes -> CheckNullifiers as a background task - ShieldedTabView reads its state from AppContext::shielded_states via refresh_from_backend_state(), never dispatching init itself - The "Initialize Shielded Wallet" button and WalletUnlockPopup are removed from ShieldedTabView (wallet unlock already calls handle_wallet_unlocked which handles everything) - Resync Notes (developer mode) still dispatches InitializeShieldedWallet as an explicit user action Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(shielded): deduplicate notes in sync to prevent double balance When the commitment tree resets (empty/corrupt) but persisted notes remain in the DB, initialize_shielded_wallet loads notes from DB while last_synced_index falls to 0. The subsequent sync rescans from position 0 and re-appends all decrypted notes — duplicating the ones already in memory from DB, doubling the balance. Fix: sync_notes() now checks if a note at the same position already exists in shielded_state.notes before appending, preventing duplicates regardless of the last_synced_index value. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(ui): add shielded diversified address table Replace the single-address display with prev/next buttons in the Shielded tab with a collapsible address table showing all generated diversified addresses. Each row shows the index, truncated bech32m address (click or Copy button to copy full address), and status (Default for index 0). The section is collapsed by default in normal mode and expanded in developer mode. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review findings — duplicate controls, perf, tab visibility - Remove duplicate sync status/buttons block in shielded_tab.rs - Fix BIP44 accounts with index > 0 being invisible (regression) - Fix early return in render_account_tabs blocking Shielded tab for new wallets - Fix double-tick of ShieldedTabView per frame when tab is active - Fix system_tab_sections O(n*m) perf by precomputing address counts - Track queue_shielded_sync via subtasks for graceful shutdown - Use HashSet for O(1) duplicate note check in sync (was O(n^2)) - Fix balance_breakdown default_open to respect developer mode - Fix is_system_category to delegate to is_visible_in_default_mode - Fix doc comment for system_tab_sections sort order claim - Narrow #[allow(dead_code)] to specific field on AccountSummary Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): replace mutex unwrap() with graceful error handling in shielded tab Replace .lock().unwrap() calls on shielded_states with .lock().ok() patterns throughout ShieldedTabView to prevent UI panics on poisoned mutex. Shows fallback messages or skips updates gracefully. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(mcp): resolve async lifetime errors for Rust 2024 edition Use spawn_blocking + block_on in dispatch_task to avoid Send bound issues with platform SDK types (DataContract/Sdk references across await points). Same pattern already used by AppState::handle_backend_task. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: fix mcp dispatch * doc: remove obsolete manual teting docs * fix(ui): include zero-balance addresses in AddressInput wallet entries Pull Core addresses from known_addresses (all derived) instead of address_balances (only funded). Balance is looked up from address_balances, defaulting to 0. Callers use with_balance_range(1..) when they need funded-only addresses. Fixes Mine dialog showing empty dropdown for fresh/zero-balance wallets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): sort AddressInput wallet entries alphabetically Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * revert: remove redundant sort in extract_wallet_entries filtered_entries() already sorts alphabetically by display_label. The sort added in extract_wallet_entries was redundant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): clarify shielded network validation logic Replace double-negation with explicit mainnet-class comparison. Documents that testnet/devnet/local share the same HRP ("tdash1z") and cannot be distinguished at the address level. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): include send-all transactions in wallet history Check both outputs and inputs when filtering transactions. Build an OutPoint→Address lookup from the wallet's own transactions to resolve input addresses. Send-all transactions (no change output) were previously dropped because no output matched known_addresses. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): use is_ours flag for transaction filtering Simplify transaction filter to use the is_ours flag instead of address matching. Fix SPV path to set is_ours=true for all wallet transactions (upstream only sets it for sends). SPV history is per-wallet, so all entries are ours by definition. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(e2e): verify is_ours flag for SPV send and receive transactions Sends funds between two wallets via SPV and asserts that both the sender (negative net_amount) and receiver (positive net_amount) have is_ours=true. Validates the fix to the SPV reconcile path that overrides the upstream library's send-only is_ours logic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(core): clear RPC error state on successful chain lock fetch When get_best_chain_lock() succeeds on the active network, explicitly clear any lingering RPC error in ConnectionStatus so the connection indicator recovers after a transient outage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(model): move is_platform_address_string from UI helpers to model layer AddressKind::detect() in the model layer depended on crate::ui::helpers for platform address detection — wrong layering. Move the function to src/model/address.rs and keep a re-export in helpers for existing callers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): add actionable guidance to address validation error messages Each validation error now tells the user what to do, not just what went wrong: "check for typos", "check you are using the correct network", "use all lowercase". Messages that already had guidance are unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): invalidate address inputs on all screens during context switch TransferScreen, AddressBalanceScreen, and ShieldedSendScreen retained stale address text from the previous network after a context switch. Add invalidate_address_input() to each and call it from change_context(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): clear password field on network switch when no config exists When switching networks, if no config entry exists for the new network (or the password is empty), the password input field now clears instead of retaining the previous network's password. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): clear validated_destination in invalidate_address_input WalletSendScreen and UnshieldCreditsScreen cleared the AddressInput widget but left validated_destination with stale validation from the previous network. Now both are cleared together. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): consume ShieldedNotesSynced to update shielded send screen state After a shielded transfer, the post-transfer note sync result was only logged but never used to update the UI. Now ShieldedNotesSynced updates the balance display, clears the pending-update banner, and appends the remaining balance to the success message. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: apply cargo +nightly fmt formatting Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(model): disambiguate Core vs Identity address detection by prefix Core addresses on Dash start with X/Y (mainnet) or y/8/7 (testnet). When the input starts with a known Core prefix, try Core first. Otherwise try Identity first, since Identity IDs use the same Base58 alphabet but don't have Core address prefixes. This fixes identity-only AddressInput mode rejecting valid Identity IDs that happened to parse as Core addresses, and eliminates the flaky test that had to skip Core-looking random identifiers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(core): show actual RPC error on Networks page instead of generic message The Networks page is a debugging tool — users need the real error text to diagnose Dash Core issues. Replace sanitized "RPC error — check Dash Core status" with the actual error from dashcore_rpc. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): resolve deadlock in clear_network_data clear_network_data() held self.conn.lock() for the transaction, then called clear_commitment_tree_tables() which tried to acquire the same mutex — classic non-reentrant mutex deadlock. UI froze on "Delete Data". Fix: scope the connection lock so it's released before the commitment tree clearing acquires it again. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): replace mutex unwrap() with graceful .ok() in shielded_sync_task The shielded_states mutex lock used .unwrap() which would panic on a poisoned mutex. Replace with .ok()? to return None gracefully. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): replace RwLock unwrap() with graceful error handling in shielded tab Replace wallets.read().unwrap() with .read().ok() and show an error label instead of panicking on a poisoned RwLock. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(spv): add debug log when overriding is_ours for receive transactions Log at debug level when SPV reports is_ours=false for a receive transaction (net_amount >= 0) to detect upstream API behavior changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(shielded): log warning on note value divergence during position dedup Use a HashMap<position, value> instead of HashSet<position> so we can detect when a re-synced note at an existing position has a different value — indicating potential data integrity issues. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * perf(model): avoid allocation in is_platform_address_string Replace to_lowercase() with eq_ignore_ascii_case() for the HRP prefix check. The HRP constants are ASCII-only so this is safe and avoids a heap allocation on every call. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(ui): remove unused label field and is_key_only from AccountSummary The label field was constructed but never read outside the builder. is_key_only() had no callers. Remove both instead of suppressing dead_code warnings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(ui): avoid cloning full AccountTab enum in tab content match Extract only the category data (clone of AccountCategory + index) before the match instead of cloning the entire enum. This avoids unnecessary allocation for the Shielded and System variants. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ui): clear validated_address on network switch in mine dialog invalidate_address_inputs() cleared the AddressInput widget but left the validated_address stale, which could reference an address from the previous network. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(model): move truncate_address to model layer and document ASCII precondition Move truncate_address from address_input.rs and shielded_tab.rs into src/model/address.rs as a parameterized public function. Both callers now delegate to the shared implementation with their own prefix/suffix lengths. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs(shielded): document why spawn_blocking trampoline is needed in queue_shielded_sync The spawn_blocking(block_on(...)) pattern is required because async methods on Arc<Self> produce non-'static futures due to a known Rust compiler limitation (rust-lang/rust#100013). Document this to prevent future removal attempts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: apply nightly rustfmt formatting Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add unified send screen requirements Full requirements for extending the Send screen to support all 14 viable Source×Destination combinations plus 13 new MCP/CLI tools. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add unified send screen architecture System layer traces for all 8 new combinations, code placement plan, send_screen decomposition strategy, MCP tools architecture (13 tools across 3 files), and 8-task work decomposition with dependency graph. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(ui): add unified send screen with all 14 source-destination combinations Wire 8 new send combinations into the Send screen routing: - Core->Shielded, Core->Identity, Platform->Shielded, Platform->Identity - Shielded->Core, Identity->Core, Identity->Platform, Identity->Identity Add Identity as 4th source type with selector dropdown, progressive disclosure (developer mode gates shielded, identities shown when loaded), and display_task_result handlers for all new backend result types. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(mcp): add 10 identity and shielded MCP tools Add identity tools: topup (core/platform), transfer, withdraw, to-address. Add shielded tools: shield-from-core, shield-from-platform, transfer, unshield, withdraw. Add resolve::qualified_identity() helper and validate_credits() for shared parameter validation. Box QualifiedIdentity in SourceSelection::Identity to fix clippy large_enum_variant warning. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add user stories and MCP tool reference for unified send Add SND-007 through SND-013 user stories for new send combinations. Add 10 new tools to MCP.md tool reference table. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(ui): add unified send screen with all 14 source-destination combinations Wire 8 new send combinations into the Send screen routing: - Core->Shielded, Core->Identity, Platform->Shielded, Platform->Identity - Shielded->Core, Identity->Core, Identity->Platform, Identity->Identity Add Identity as 4th source type with selector dropdown, progressive disclosure (developer mode gates shielded, identities shown when loaded), and display_task_result handlers for all new backend result types. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(mcp): add 10 identity and shielded MCP tools Add identity tools: topup (core/platform), transfer, withdraw, to-address. Add shielded tools: shield-from-core, shield-from-platform, transfer, unshield, withdraw. Add resolve::qualified_identity() helper and validate_credits() for shared parameter validation. Box QualifiedIdentity in SourceSelection::Identity to fix clippy large_enum_variant warning. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: resolve merge conflicts with v1.0-dev Restore address_input.rs from v1.0-dev (includes change/system address filtering). Fix render_address_table calls to pass account_filter parameter. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #798 review findings across send screen and MCP tools Send screen (C01/C02/C03/C05): - Validate shielded destination before dispatching shield routes - Check highest-address platform balance for Platform->Shielded - Subtract estimated fee from identity source available balance - Improve identity-not-found error to be actionable without navigation MCP tools (C09/C10/C15): - Add INTENTIONAL comments for tools that skip SPV …
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
TaskErrorvariants:LockPoisoned,Database,CoreRpc,P2PP2PErrordomain type for peer-to-peer network errorsWalletErrorwithKeyDerivationandSighashvariantsDashPayErrorwithQueryCreation,CryptoKeyParsing,PrivateKeyResolutionvariants; dropClone/PartialEqderives (unused)ConfigError::SaveErrorfor file write operations.map_err(|e| e.to_string())with typed error propagation acrossbackend_task/,ui/,model/,database/,context/,spv/,config,components/From<PoisonError<T>>forTaskError::LockPoisoneddashcore_rpc::ErrortoTaskError::CoreRpcinstead ofGenericTaskError::fromconversions~50 callsites migrated to typed errors in this PR. ~854 remaining callsites still compile via
From<String>bridge for incremental follow-up.Closes #660
Test plan
cargo build --all-featurescompiles cleanlycargo clippy --all-features --all-targets -- -D warningspassescargo test --all-features --workspace— 72 tests + 3 doc-tests passMessageBannershows user-friendly text, not raw error strings🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Bug Fixes
UI
New Features
Documentation