backport: merge v1.0-dev (including extracted non-ZK fixes) back into zk#644
Open
backport: merge v1.0-dev (including extracted non-ZK fixes) back into zk#644
Conversation
…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>
…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: 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>
…tion (#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>
…ection() (#643) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…641) * feat(ui): add Auto Update button for dashmate RPC password Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add manual test scenarios for dashmate auto-update Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): show error message when dashmate Auto Update fails Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): use MessageBanner for dashmate Auto Update errors Replace custom inline Frame-based error display with the codebase's standard MessageBanner::set_global() pattern. The banner is rendered centrally by island_central_panel() and auto-dismisses after a default duration, providing a consistent user experience. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
#646) * fix(wallet): use mode-aware broadcast for platform funding in SPV mode fund_platform_address_from_wallet_utxos() called core_client.send_raw_transaction() directly, bypassing the mode-aware broadcast_raw_transaction() helper. This broke core-to-platform transfers in SPV mode where no RPC connection is available. Changes: - Replace direct RPC broadcast with self.broadcast_raw_transaction() which routes to SPV manager in SPV mode and Core RPC in RPC mode - Guard UTXO reload fallback with core_backend_mode() check: only attempt RPC-based reload_utxos in RPC mode; return error in SPV mode where wallet state is authoritative (matching register_identity.rs and top_up_identity.rs) - Remove unused dash_sdk::dashcore_rpc::RpcApi import Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): use mode-aware broadcast for platform funding in SPV mode fund_platform_address_from_wallet_utxos() called core_client.send_raw_transaction() directly, bypassing the mode-aware broadcast_raw_transaction() helper. This broke core-to-platform transfers in SPV mode where no RPC connection is available. Changes: - Replace direct RPC broadcast with self.broadcast_raw_transaction() which routes to SPV manager in SPV mode and Core RPC in RPC mode - Guard UTXO reload fallback with core_backend_mode() check: only attempt RPC-based reload_utxos in RPC mode; return error in SPV mode where wallet state is authoritative (matching register_identity.rs and top_up_identity.rs) - Store asset lock transaction in DB after broadcast so the SPV finality listener can retrieve it when processing InstantLock/ChainLock events (without this, the finality proof is never populated and the wait loop times out after 5 minutes) - Remove unused dash_sdk::dashcore_rpc::RpcApi import Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(wallet): simplify reload_utxos signature and callers - Change reload_utxos from (Client, Network, Option<AppContext>) to (&AppContext), returning Result<bool, String> (true = UTXOs changed) - SPV mode: no-op returning Ok(false) instead of error - RPC mode: acquire core_client internally with map_err (SEC-03 fix) - Callers skip retry when reload reports no changes - Replace inline tokio::select! timeout loop in fund_platform_address with shared wait_for_asset_lock_proof helper (SEC-04 fix) - Add mode-aware post-timeout recovery (RPC: refresh_wallet_info, SPV: tracing::warn about automatic reconciliation) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address review findings CODE-01, CODE-02, CODE-04 - CODE-01: Move store_asset_lock_transaction before broadcast to eliminate race where SPV finality listener fires before the DB row exists. Clean up DB row and finality tracking on broadcast failure. - CODE-02: Guard DB persistence in reload_utxos with `if changed` to skip empty-set iteration when nothing changed. - CODE-04: Renumber step comments sequentially (1-9, no gaps). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…setup (#640) * feat(scripts): add configure-local.sh for dashmate network setup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(scripts): address PR review comments on configure-local.sh - Hide RPC password from console output, show "(found)" instead - Support DASHMATE_CMD env var for custom dashmate command (e.g. DASHMATE_CMD="yarn dashmate" ./configure-local.sh) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(scripts): quote RPC password in .env output Wrap the RPC password value in single quotes when writing to .env to prevent mis-parsing if the password contains #, spaces, or $. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…on input count (#636) * fix(wallet): calculate asset lock tx fee dynamically based on input count Replace hardcoded 3000 duff fee with dynamic fee calculation that accounts for actual number of inputs. Estimates tx size using standard component sizes (P2PKH input ~148B, output ~34B, header ~10B, payload ~60B) and uses max(3000, estimated_size) to always meet the min relay fee. Properly handles fee shortfall when allow_take_fee_from_amount is set, and returns clear error messages for insufficient funds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add manual test scenarios for asset lock fee fix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(wallet): extract asset lock fee calculation to pure function Extract fee/amount/change computation from the inline logic in asset_lock_transaction_from_private_key() into a standalone calculate_asset_lock_fee() function. Uses an iterative approach that fixes a bug where the fee was computed assuming a change output existed (based on the initial 3000-duff estimate). When the real fee eliminated the change, the code overestimated by 34 bytes and could trigger a false insufficient-funds error on edge cases with many inputs. Also removes stale edge case E3 from manual test scenarios (referenced a database refactor not in this PR). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test(wallet): add unit tests for calculate_asset_lock_fee() Covers basic fee scenarios (minimum fee, scaling with inputs, exact change, fee-from-amount, insufficient funds) and two regression tests that prove the bug fixed in the previous commit — false insufficient funds when change disappears under the real fee with many inputs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): overflow guard, dust threshold, and constant for fee calc - Use checked_add for requested_amount + fee to prevent u64 overflow - Add DUST_THRESHOLD (546 duffs) — change below this is absorbed into the fee instead of creating a non-standard dust output - Replace hardcoded 3_000u64 with MIN_ASSET_LOCK_FEE constant in caller Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update docs/ai-design/2026-02-24-asset-lock-fee-fix/manual-test-scenarios.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(wallet): correct UTXO rollback type mismatch in asset lock fee error path The rollback code used `extend()` which expected `(Address, HashMap<OutPoint, TxOut>)` but received `(OutPoint, (TxOut, Address))` from `take_unspent_utxos_for()`. Re-group UTXOs by address using entry API to match `self.utxos` structure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…d signed (#645) * fix(wallet): calculate asset lock tx fee dynamically based on input count Replace hardcoded 3000 duff fee with dynamic fee calculation that accounts for actual number of inputs. Estimates tx size using standard component sizes (P2PKH input ~148B, output ~34B, header ~10B, payload ~60B) and uses max(3000, estimated_size) to always meet the min relay fee. Properly handles fee shortfall when allow_take_fee_from_amount is set, and returns clear error messages for insufficient funds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add manual test scenarios for asset lock fee fix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Initial plan * fix(wallet): defer UTXO removal until asset lock tx is fully built and signed Previously, `asset_lock_transaction_from_private_key` called `take_unspent_utxos_for` which immediately removed selected UTXOs from `Wallet.utxos`. Since fee recalculation and signing happen afterward, any failure at those steps (fee shortfall, missing private key, change address derivation error) would permanently drop UTXOs — especially dangerous in SPV mode where there is no Core RPC reload fallback. Fix: - Add `select_unspent_utxos_for` (`&self`, non-mutating) that performs the same UTXO selection logic without removing anything. - Add `remove_selected_utxos` (`&mut self`) for explicit removal. - Refactor `take_unspent_utxos_for` to delegate to these two methods (no behavior change for existing callers). - In `asset_lock_transaction_from_private_key`, use `select_unspent_utxos_for` for selection and only call `remove_selected_utxos` after the full tx is built and signed. Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor(wallet): consolidate UTXO removal, DB persistence, and balance recalc into remove_selected_utxos Previously, every backend task caller had to manually: (1) remove UTXOs from the in-memory map, (2) drop them from the database, and (3) recalculate affected address balances. This was error-prone — the payment transaction builders were missing the balance recalculation entirely. Now `remove_selected_utxos` accepts an optional `&AppContext` and handles all three steps atomically. The redundant cleanup blocks in 5 backend task callers are removed. Also applies the safe select-then-commit UTXO pattern to `build_standard_payment_transaction` and `build_multi_recipient_payment_transaction`, fixing the same UTXO-loss-on-signing-failure bug that was previously fixed only for asset lock transactions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
* fix(wallet): calculate asset lock tx fee dynamically based on input count Replace hardcoded 3000 duff fee with dynamic fee calculation that accounts for actual number of inputs. Estimates tx size using standard component sizes (P2PKH input ~148B, output ~34B, header ~10B, payload ~60B) and uses max(3000, estimated_size) to always meet the min relay fee. Properly handles fee shortfall when allow_take_fee_from_amount is set, and returns clear error messages for insufficient funds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add manual test scenarios for asset lock fee fix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Initial plan * fix(wallet): defer UTXO removal until asset lock tx is fully built and signed Previously, `asset_lock_transaction_from_private_key` called `take_unspent_utxos_for` which immediately removed selected UTXOs from `Wallet.utxos`. Since fee recalculation and signing happen afterward, any failure at those steps (fee shortfall, missing private key, change address derivation error) would permanently drop UTXOs — especially dangerous in SPV mode where there is no Core RPC reload fallback. Fix: - Add `select_unspent_utxos_for` (`&self`, non-mutating) that performs the same UTXO selection logic without removing anything. - Add `remove_selected_utxos` (`&mut self`) for explicit removal. - Refactor `take_unspent_utxos_for` to delegate to these two methods (no behavior change for existing callers). - In `asset_lock_transaction_from_private_key`, use `select_unspent_utxos_for` for selection and only call `remove_selected_utxos` after the full tx is built and signed. Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor(wallet): consolidate UTXO removal, DB persistence, and balance recalc into remove_selected_utxos Previously, every backend task caller had to manually: (1) remove UTXOs from the in-memory map, (2) drop them from the database, and (3) recalculate affected address balances. This was error-prone — the payment transaction builders were missing the balance recalculation entirely. Now `remove_selected_utxos` accepts an optional `&AppContext` and handles all three steps atomically. The redundant cleanup blocks in 5 backend task callers are removed. Also applies the safe select-then-commit UTXO pattern to `build_standard_payment_transaction` and `build_multi_recipient_payment_transaction`, fixing the same UTXO-loss-on-signing-failure bug that was previously fixed only for asset lock transactions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address audit findings from PR #645 review - Add checked arithmetic to UTXO selection (amount + fee overflow safety) - Replace hardcoded fee in single-UTXO path with calculate_asset_lock_fee - Add UTXO selection retry when real fee exceeds initial estimate - Document write-lock invariant on select_unspent_utxos_for - Replace .unwrap() with .map_err() on wallet write locks - Restrict Database::shared_connection visibility to pub(crate) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(wallet): simplify remove_selected_utxos to take &Database + Network directly Replace Option<&AppContext> with concrete dependencies (&Database, Network), removing the need for take_unspent_utxos_for. Extract balance recalculation into a private helper reused by both remove_selected_utxos and the existing AppContext-based wrapper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
… mode (#638) * feat(wallet): add Mine Blocks dialog for Regtest/Devnet dev mode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add manual test scenarios for mine blocks dialog Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): close Mine Blocks dialog after Cancel/Mine click The dialog stayed open (in a broken state) after clicking Mine or Cancel because the local `open` variable was written back to `is_open` after the dialog state had already been reset. Pass `is_open` directly to egui's `.open()` and use a separate `close` flag for button-triggered dismissal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address audit findings for Mine Blocks dialog - Wrap `generate_to_address` in `spawn_blocking` to avoid blocking the async runtime thread (HIGH) - Replace `.expect()` on core client lock with `.map_err()?` for graceful error handling instead of panic (HIGH) - Cap block count at 1000 to prevent resource exhaustion on the Core node (HIGH) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): filter non-numeric input in Mine Blocks block count field Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address review comments on Mine Blocks dialog - Replace assume_checked() with require_network() for address validation (CodeRabbit #2) - Use styled Frame-with-dismiss error display matching Send dialog pattern (CodeRabbit #3) - Don't open dialog when no wallet selected; show MessageBanner instead (CodeRabbit #4) - Extract shared load_bip44_external_addresses() helper to eliminate near-duplicate code between mine and receive dialogs (CodeRabbit #5) - Add backend-side network guard (Regtest/Devnet) for defense-in-depth (CodeRabbit #6) - Rename shadowed ctx binding to refresh_ctx for clarity (CodeRabbit #7) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address remaining review comments on Mine Blocks dialog - Change MineBlocksSuccess(usize) to MineBlocksSuccess(u64) for type consistency with block_count parameter (Claudius #5) - Align dialog close pattern with Send/Receive: use local `open` variable for egui X button, reset state inside closure for Cancel/Mine buttons (Claudius #6) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…637) * feat(ui): show nonce column for Platform Payment addresses Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add manual test scenarios for address nonce column Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): reset sort column when switching to platform payment account When toggling to a Platform Payment account view, the sort column could remain set to UTXOs or TotalReceived which are not rendered for that account type. This resets it to Balance (descending) in that case. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * perf(wallet): gate platform info lookup on account type Skip get_platform_address_info() for non-platform addresses to avoid unnecessary linear scans in the fallback path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ister_addresses params (#649) All 12 wallet functions that took `register_addresses: Option<&AppContext>` now require `&AppContext` unconditionally (placed first after `&mut self` per project convention). This eliminates silently skipped UTXO removal and address registration when callers forget to pass Some(...). For `identity_authentication_ecdsa_public_keys_data_map`, a separate `register_addresses: bool` flag controls whether addresses are registered in the DB, preserving the search-loop optimization in load_identity.rs. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…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>
…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: 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>
…tion (#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>
Resolve conflicts preserving shielded pool features from HEAD while adopting v1.0-dev improvements: refactored asset lock fee calculation, mine dialog hardening, network validation, and API signature updates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…xtract/all-merged
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>
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>
Tests were using the production database (data.db) which risked data corruption and made tests depend on prior app setup. Add a `testing` feature flag that swaps AppState::new() to use an in-memory SQLite database via the existing create_test_database() helper. No test files need changes since `--all-features` enables the testing feature automatically. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
… duplicate errors (#654) * fix(ui): fix message banner icons, dismiss button, text wrapping, and duplicate errors - Change error icon from ❌ to ⛔ to avoid confusion with dismiss button - Use ❌ as styled dismiss button (clickable label, pointer cursor on hover) - Fix success icon (✅) and info icon (💬) to render correctly in Noto Sans - Wrap long banner text at word boundaries instead of overflowing window - Left-align text within the wrapping area - Remove duplicate error display in AddNewIdentityScreen — delegate to global MessageBanner - Route local validation errors through MessageBanner::set_global for consistent styling Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): left-align banner text and sort imports Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): clamp banner text width to zero instead of 100px minimum Prevents row overflow in very narrow windows by allowing text_width to shrink to zero rather than forcing a 100px floor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): reserve space for countdown annotation in banner text width Account for annotation width (e.g. "(5s)" countdown) when calculating text_width so it doesn't overlap long messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test(ui): update banner tests for new icons and dismiss button Update kittest assertions to match new dismiss button (❌ emoji label instead of "x" small_button) and new icon mappings (⛔ error, ✅ success, 💬 info). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address PR review comments on banner layout and error handling - Fix comment: icon is not actually "fixed width" (#2850192960) - Measure annotation width dynamically instead of hard-coded 52px (#2850192972) - Prevent validation errors from re-pushing to global banner each frame by tracking last-sent message (#2850192953) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): reduce annotation width estimate for banner layout Lower per-character width multiplier from 0.55 to 0.4 to match actual rendered width of digit/paren annotation strings like "(5s)". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* fix(wallet): calculate asset lock tx fee dynamically based on input count Replace hardcoded 3000 duff fee with dynamic fee calculation that accounts for actual number of inputs. Estimates tx size using standard component sizes (P2PKH input ~148B, output ~34B, header ~10B, payload ~60B) and uses max(3000, estimated_size) to always meet the min relay fee. Properly handles fee shortfall when allow_take_fee_from_amount is set, and returns clear error messages for insufficient funds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(ui): add sync status panel to wallet screen Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add manual test scenarios for asset lock fee fix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add manual test scenarios for sync status panel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Initial plan * fix(wallet): defer UTXO removal until asset lock tx is fully built and signed Previously, `asset_lock_transaction_from_private_key` called `take_unspent_utxos_for` which immediately removed selected UTXOs from `Wallet.utxos`. Since fee recalculation and signing happen afterward, any failure at those steps (fee shortfall, missing private key, change address derivation error) would permanently drop UTXOs — especially dangerous in SPV mode where there is no Core RPC reload fallback. Fix: - Add `select_unspent_utxos_for` (`&self`, non-mutating) that performs the same UTXO selection logic without removing anything. - Add `remove_selected_utxos` (`&mut self`) for explicit removal. - Refactor `take_unspent_utxos_for` to delegate to these two methods (no behavior change for existing callers). - In `asset_lock_transaction_from_private_key`, use `select_unspent_utxos_for` for selection and only call `remove_selected_utxos` after the full tx is built and signed. Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor(wallet): consolidate UTXO removal, DB persistence, and balance recalc into remove_selected_utxos Previously, every backend task caller had to manually: (1) remove UTXOs from the in-memory map, (2) drop them from the database, and (3) recalculate affected address balances. This was error-prone — the payment transaction builders were missing the balance recalculation entirely. Now `remove_selected_utxos` accepts an optional `&AppContext` and handles all three steps atomically. The redundant cleanup blocks in 5 backend task callers are removed. Also applies the safe select-then-commit UTXO pattern to `build_standard_payment_transaction` and `build_multi_recipient_payment_transaction`, fixing the same UTXO-loss-on-signing-failure bug that was previously fixed only for asset lock transactions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address audit findings from PR #645 review - Add checked arithmetic to UTXO selection (amount + fee overflow safety) - Replace hardcoded fee in single-UTXO path with calculate_asset_lock_fee - Add UTXO selection retry when real fee exceeds initial estimate - Document write-lock invariant on select_unspent_utxos_for - Replace .unwrap() with .map_err() on wallet write locks - Restrict Database::shared_connection visibility to pub(crate) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: minimal improvement in conn status tooltip * refactor(wallet): simplify remove_selected_utxos to take &Database + Network directly Replace Option<&AppContext> with concrete dependencies (&Database, Network), removing the need for take_unspent_utxos_for. Extract balance recalculation into a private helper reused by both remove_selected_utxos and the existing AppContext-based wrapper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address remaining audit findings from code review - Refresh platform sync info cache after wallet refresh completes - Add broadcast failure cleanup in create_asset_lock (remove stale finality tracking entries, replace mutex .unwrap() with .map_err()) - Replace .expect() with proper error propagation in signing loops - Use i128 for fee logging subtraction to prevent overflow - Renumber step comments sequentially after refactoring Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): refresh sync info cache after platform balance fetch The PlatformAddressBalances task result handler updated wallet balances but did not refresh the platform_sync_info cache, causing the UI to display "never synced" until the wallet was reselected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(ui): make sync status panel collapsible and dev-mode only The Core/Platform sync status panel is now hidden by default and only visible when developer mode is enabled. It uses a collapsible header so developers can expand/collapse it as needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address PR #642 review findings - Centralize wallet selection via set_selected_hd_wallet() helper to keep platform_sync_info cache consistent across all code paths - Add tracing::warn! for Mutex poisoning in asset lock cleanup paths - Fix misleading comment about wallet refresh on broadcast failure - Remove TS-25 from manual test scenarios (not part of this PR) Refs: #657 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): extract shared SPV phase summary and enrich tooltip Consolidate duplicated SPV sync phase formatting into a shared `spv_phase_summary()` function in `connection_status.rs`. The wallet screen now uses this shared function instead of its own copy. The network screen retains its richer operator-facing formatter. The connection indicator tooltip now shows detailed sync progress (e.g. "SPV: Headers: 12345 / 27000 (45%)") instead of bare "SPV: Syncing" when in SPV mode. Also adjust refresh polling rates: 4s when connected, 1s when disconnected (was 10s/2s). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: apply nightly rustfmt formatting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address second round of PR review comments - Update test scenarios TS-07 through TS-11 and TS-23 to reflect new SPV phase format: "Headers: C / T (NN%)" instead of "Headers NN%" - Add Masternodes phase to TS-23 progression - Add developer mode precondition to test scenarios - Fix tooltip showing "syncing..." when SPV is fully synced (Running) - Update stale throttle comment to reflect new refresh rates Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
…rmSyncMode (#635) * refactor(wallet): simplify platform sync by removing PlatformSyncMode Remove the PlatformSyncMode enum (Auto/ForceFull/TerminalOnly) and terminal sync logic (apply_recent_balance_changes, last_terminal_block, last_full_sync_balance). The SDK now handles incremental sync internally via AddressProvider::current_balances() and last_sync_height(). Key changes: - Remove PlatformSyncMode enum from backend_task::wallet - Simplify fetch_platform_address_balances to use new SDK API with stored state (with_stored_state, current_balances, last_sync_height) - Change CoreTask::RefreshWalletInfo to use bool instead of Option<PlatformSyncMode> - Remove last_full_sync_balance from PlatformAddressInfo - Simplify database sync info to 2-tuple (timestamp, height) - Remove set_last_terminal_block from database - Simplify RefreshMode enum (remove PlatformFull, PlatformTerminal, CoreAndPlatformFull, CoreAndPlatformTerminal variants) Note: requires updated dash-sdk with new sync_address_balances API. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: update platform SDK to rev 0fa82e6652 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add manual test scenarios for platform sync simplification Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address PR #635 audit findings and extract broadcast helper - DB schema v28: drop obsolete columns (last_terminal_block, last_full_sync_balance), rename last_platform_sync_checkpoint → last_platform_sync_height, with SQLite ≥3.35 runtime check - Store asset lock TX before broadcast to prevent SPV InstantSend race - Defer UTXO removal until after successful broadcast - Replace .unwrap() on RwLock with .map_err() to avoid panics - Remove unused _is_sync_operation param and set_platform_address_info_from_sync wrapper - Fix redundant let-mut rebinding in fetch_platform_address_balances - Extract broadcast_and_commit_asset_lock() on AppContext to consolidate the store→broadcast→cleanup→UTXO-removal pattern from 5 code paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(db): schema v28 — drop obsolete sync columns and clean up DB interface (#653) - Bump DEFAULT_DB_VERSION 27 → 28 - Drop last_terminal_block from wallet table (unused after sync simplification) - Drop last_full_sync_balance from platform_address_balances table - Rename last_platform_sync_checkpoint → last_platform_sync_height - Add runtime SQLite ≥3.35 check (required for DROP COLUMN) - Idempotent migration: checks column existence before each ALTER - Remove unused _is_sync_operation param from set_platform_address_info() - Remove set_platform_address_info_from_sync() wrapper - Fix redundant let-mut rebinding in fetch_platform_address_balances Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * revert(db): remove schema v28 migration from this branch The v28 schema changes (drop obsolete sync columns, rename last_platform_sync_checkpoint → last_platform_sync_height) will be applied separately and should not ship on this branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review comments — remove dead code and document risks - Remove dead `_is_sync_operation` parameter from `Database::set_platform_address_info` and all call sites (Finding #6) - Add doc comment on `set_platform_sync_info` explaining column name drift: `last_platform_sync_checkpoint` now stores SDK sync height (Finding #4) - Remove trivial `set_platform_address_info_from_sync` delegate and update callers to use `set_platform_address_info` directly (Finding #7) - Combine unnecessary `let provider` + `let mut provider = provider` rebinding into single `let mut` block (Finding #10) - Document UTXO selection race window on `broadcast_and_commit_asset_lock`: std::sync::RwLock guard is !Send so it cannot span async broadcast Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: apply nightly fmt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…rged # Conflicts: # Cargo.lock # Cargo.toml # src/backend_task/core/mod.rs # src/backend_task/wallet/fetch_platform_address_balances.rs # src/context/wallet_lifecycle.rs # src/database/wallet.rs # src/model/wallet/mod.rs # src/ui/wallets/wallets_screen/mod.rs
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>
lklimek
changed the title
lklimek
changed the title
Contributor
Author
|
@copilot please fix failing tests. |
Contributor
* 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>
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
Integrates all backported non-ZK improvements from the
zkbranch intov1.0-dev, then merges the updatedv1.0-devhistory back intozk. This eliminates divergence between branches and ensures thezkbranch benefits from all code review improvements made during the backport process.What happened
The
zkbranch accumulated 9 independent improvements alongside zero-knowledge shielded pool work. We extracted each into standalone PRs targetingv1.0-dev, where they went through independent code review and CI. Reviewers caught issues and improvements were made — those fixes now flow back intozkthrough this PR.Extraction PRs — Final Status
PlatformSyncMode, use SDK incremental sync (rev0fa82e6)is_platform_address()helper (deemed unnecessary)configure-local.shfor dashmate network setupArc<Mutex<Connection>>withshared_connection()8 of 9 merged, 1 closed as unnecessary.
Improvements from code review (flowing back to
zk)These fixes were made during the backport review process and are now included:
Option<&AppContext>with&AppContextin register_addressesMerge conflict resolution
The final
v1.0-devmerge required resolving conflicts in 8 files. Each was inspected individually:feat/zkSDK branch (required for ZK support)v1.0-devimprovements: removed unused_is_sync_operationparam, better doc comments, cleaner variable names, removed redundantset_platform_address_info_from_syncwrapperspv_phase_summaryfree function while preserving ZK shielded notes/nullifiers displayset_platform_address_infosignatureSDK dependency
This branch uses
feat/zkSDK. Thev1.0-devbranch uses rev0fa82e6652097d17a700d8bcc006d6b2aa922c6ewhich backports incremental sync without ZK. No SDK conflict — thefeat/zkbranch is a superset.Test plan
--ours)zkbranch builds and passes tests after merging this PRdocs/ai-design/2026-02-24-*/manual-test-scenarios.mdClaude Session Info
Session ID:
88c54f17-2dd9-438c-abdd-43ccacf179bbTo resume this session:
🤖 Co-authored by Claudius the Magnificent AI Agent