Skip to content

backport: merge v1.0-dev (including extracted non-ZK fixes) back into zk#644

Merged
QuantumExplorer merged 82 commits into
zkfrom
zk-extract/all-merged
Mar 22, 2026
Merged

backport: merge v1.0-dev (including extracted non-ZK fixes) back into zk#644
QuantumExplorer merged 82 commits into
zkfrom
zk-extract/all-merged

Conversation

@lklimek
Copy link
Copy Markdown
Contributor

@lklimek lklimek commented Feb 24, 2026

Summary

Integrates all backported non-ZK improvements from the zk branch into v1.0-dev, then merges the updated v1.0-dev history back into zk. This eliminates divergence between branches and ensures the zk branch benefits from all code review improvements made during the backport process.

What happened

The zk branch accumulated 9 independent improvements alongside zero-knowledge shielded pool work. We extracted each into standalone PRs targeting v1.0-dev, where they went through independent code review and CI. Reviewers caught issues and improvements were made — those fixes now flow back into zk through this PR.

Extraction PRs — Final Status

PR Status Description
#635 ✅ Merged Remove PlatformSyncMode, use SDK incremental sync (rev 0fa82e6)
#636 ✅ Merged Dynamic asset lock tx fee calculation
#637 ✅ Merged Nonce column for Platform Payment addresses
#638 ✅ Merged Mine Blocks dialog (devnet/regtest, dev mode)
#639 ❌ Closed is_platform_address() helper (deemed unnecessary)
#640 ✅ Merged configure-local.sh for dashmate network setup
#641 ✅ Merged Auto-update dashmate RPC password
#642 ✅ Merged Sync status panel (SPV phases, platform timestamps)
#643 ✅ Merged Arc<Mutex<Connection>> with shared_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:

Merge conflict resolution

The final v1.0-dev merge required resolving conflicts in 8 files. Each was inspected individually:

  • Cargo.toml/lock — Kept feat/zk SDK branch (required for ZK support)
  • Source files — Took v1.0-dev improvements: removed unused _is_sync_operation param, better doc comments, cleaner variable names, removed redundant set_platform_address_info_from_sync wrapper
  • wallets_screen — Manual merge: adopted v1.0-dev's collapsing header + spv_phase_summary free function while preserving ZK shielded notes/nullifiers display
  • context/shielded.rs — Updated call site to match reduced set_platform_address_info signature

SDK dependency

This branch uses feat/zk SDK. The v1.0-dev branch uses rev 0fa82e6652097d17a700d8bcc006d6b2aa922c6e which backports incremental sync without ZK. No SDK conflict — the feat/zk branch is a superset.

Test plan

  • All 8 merged extraction PRs passed CI independently
  • Merge conflicts resolved with inspection (not blind --ours)
  • Verify zk branch builds and passes tests after merging this PR
  • Run manual test scenarios from docs/ai-design/2026-02-24-*/manual-test-scenarios.md

Note: Pre-existing build errors from ZK shielded module (model::wallet::shielded not found) are expected — those types live on the zk branch and will resolve when this PR merges.


Claude Session Info

Session ID: 88c54f17-2dd9-438c-abdd-43ccacf179bb

To resume this session:

claude --resume 88c54f17-2dd9-438c-abdd-43ccacf179bb

🤖 Co-authored by Claudius the Magnificent AI Agent

lklimek and others added 6 commits February 24, 2026 09:11
…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>
@lklimek lklimek self-assigned this Feb 24, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Feb 24, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)
  • master
  • v1.0-dev

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d1746aca-3866-4532-8fe6-28b8359cd8c3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch zk-extract/all-merged
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

lklimek and others added 22 commits February 24, 2026 12:13
…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>
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>
Claudius-Maginificent and others added 24 commits March 10, 2026 15:00
…t component (#709)

* feat(security): add Secret type with zeroize-on-drop and PasswordInput component (#705, #707)

Introduce `Secret` wrapper type that zeroes sensitive strings on drop with
best-effort mlock via the `region` crate, and a reusable `PasswordInput`
component with hold-to-reveal eye icon for masked text entry.

- Secret type: zeroize-on-drop, mlock, redacted Debug, constant-time PartialEq,
  no Display/Deref (explicit expose_secret() access pattern)
- PasswordInput: masked by default, eye icon with hold-to-reveal, builder API
- Migrate all 12+ password/private-key input screens to use PasswordInput
- Fix 5 screens that displayed passwords/keys in plaintext (no masking)
- Wrap all WIF display values in Secret for zeroize-on-drop protection
- Simplify ScreenWithWalletUnlock trait from 4 methods to 1
- Remove manual .zeroize() calls (Secret handles it automatically)

Closes #705
Closes #707

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(security): harden Secret type against reallocation leaks

- Implement egui::TextBuffer for Secret, eliminating expose_secret_mut()
  escape hatch — all text mutations now go through controlled methods
  that detect pointer changes and re-mlock after reallocation
- Track heap pointer via locked_ptr field; relock_if_moved() re-locks
  the new buffer when String reallocates during insert_text/replace_with
- Fix Clone and From<&str> to pre-allocate with correct capacity,
  avoiding intermediate unprotected String copies
- Add tracing::debug! on mlock failure (was previously silent)
- Update PartialEq doc to honestly describe length-leak limitation
- Zeroize old content in TextBuffer::clear() and replace_with()

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: post-merge cleanup after v1.0-dev integration

Replace Zeroizing<String> with Secret for private key WIF values in
address_table, dialogs, and mod.rs. Remove stale TODO about PasswordInput.
Add missing dark_mode binding in SK unlock dialog.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(security): harden Secret against memory leaks and egui undo history

- SEC-001: Disable egui Undoer (max_undos=0) in PasswordInput to prevent
  plaintext copies in undo history
- SEC-003: Zeroize source string in new() before freeing old allocation
- SEC-004: Pre-allocate full page (4096 bytes) to prevent reallocations
- SEC-005: Zero trailing capacity after delete_char_range
- SEC-006: Custom Drop zeroes full 0..capacity (not just 0..len)
- SEC-007: Document .password(true) requirement in Secret doc comment
- CODE-001: Enforce PAGE_SIZE minimum in with_capacity()
- CODE-004: Deduplicate From<&str> and Clone to delegate to new()

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: explain PAGE_SIZE rationale in Secret (mlock granularity, capacity, leak prevention)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(security): eliminate plaintext String leaks at Secret callsites

Callers of Secret/PasswordInput were copying secret material into plain
String via .to_string()/.trim().to_string(), creating unzeroized heap
allocations that undermine the Secret type's guarantees. Refactored to
pass &str references directly since downstream APIs (open, from_wif,
from_hex, hex::decode) all accept &str. Added Secret::len() method.

Addresses PR #709 review comments CMT-001 through CMT-007.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(ui): request repaint on reveal state transition to prevent 1-frame plaintext leak

Previously request_repaint() only fired while revealing was true,
missing the true→false transition. Plaintext could remain visible
for one extra frame after mouse release until an unrelated repaint.

Addresses PR #709 review comment CMT-008.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* style: fix formatting for stable rustfmt

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(security): use zeroize crate for capacity wipe, rename PAGE_SIZE to DEFAULT_CAPACITY

- Drop impl now uses slice.zeroize() instead of ptr::write_bytes to
  prevent the compiler from optimizing away the security-sensitive wipe.
- Renamed PAGE_SIZE to DEFAULT_CAPACITY since 4096 is a buffer size
  choice, not necessarily the OS page size on all platforms.

Addresses PR #709 review comments from Copilot.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(security): harden Secret and callsites from review triage

Address 12 findings from grumpy-review triage:

Security (secret.rs):
- SEC-004: use volatile zeroize instead of ptr::write_bytes in delete_char_range
- SEC-006: Clone via (*self.inner).clone() instead of .to_string()
- SEC-007: CompareOnlyPtr newtype enforces Send/Sync invariant at type level
- SEC-002: compile-time needs_drop assertion + ignored runtime Drop test
- CODE-005: add Secret::trimmed() method

Callsite fixes:
- SEC-001: zeroize seed phrase Vec<String> on reset and Drop
- CODE-001: use take_secret() instead of Secret::new(text().to_string())
- CODE-002: direct &str ref for TextEdit, comment on clipboard copy
- CODE-006: derive Default instead of Secret::new(String::new())
- CODE-007: pass s directly to Secret::new() in set_text

Convention fixes:
- PROJ-002: make dashmate_password_input private
- PROJ-005: request focus once on open, not every frame

Add INTENTIONAL comments for 4 accepted risks (SEC-003, SEC-005,
CODE-003, PROJ-001).

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>
* ci: add Claude Code review workflow with grumpy-review skill

Adds a GitHub Actions workflow that triggers automated code review
when a "claude review" label is applied to a PR. Uses the claudius
grumpy-review and check-pr-comments skills via MCP-first approach.

- Triggers on label application and subsequent pushes
- Checks/resolves previously addressed review threads
- Approves and removes label when all comments resolved
- Falls back to opus model when CLAUDE_MODEL var is not set
- Skips gracefully when OAuth token is not configured

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: rename label to claudius-review, add trigger hint to workflow name

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: rename workflow to Claude, delegate review to claudius sub-agent

- Workflow name: "Claude (label: claudius-review)"
- Uses --agent claudius:claudius to run as Claudius directly
- Persona instructions in prompt for PR comment tone

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: fix permissions, reorder review flow, post MEDIUM+ only

- Add issues: write permission for label removal (CMT-001)
- Reorder flow: check existing → resolve fixed → fresh review → post MEDIUM+ → remove label → approve if clean
- Always remove claudius-review label (human-triggered each time)
- Approve only when no unresolved comments remain after full flow

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Update .github/workflows/claude-code-review.yml

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* ci: move label removal to always-run cleanup step

Label removal was inside the Claude prompt, so timeouts or failures
would leave it stuck and retrigger on every push. Now handled by a
dedicated step with if: always().

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: only remove label on successful review

Keep claudius-review label on failure/timeout so subsequent pushes
retrigger the review automatically until it completes successfully.

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>
* ci: add --allowedTools for CI tool permissions

Bash commands require approval in default permission mode, which is
impossible in CI. Add allowedTools whitelist for gh, git, python3,
MCP tools, and code reading tools.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: tighten allowedTools to review-specific commands

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: add Write, Edit, Task and git tools needed by grumpy-review

The grumpy-review skill needs Write/Edit for report generation,
Task* for spawning sub-agents, git show/rev-parse for comparison,
and cat for reading schema files.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* ci: add path filters to clippy and tests workflows

Only trigger clippy and test workflows when Rust source files,
Cargo manifests, Cargo.lock, or the workflow file itself changes.
Avoids unnecessary CI runs on docs-only or config-only changes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: include .cargo/config.toml in path filters

Rustflags and linker config changes can affect clippy and test outcomes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* ci: add claudash plugin for Dash Platform domain knowledge

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: load dash-platform skill in review prompt

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: broaden allowedTools based on actual CI failures

Add missing tools that Claude needed during review:
- Bash(git *) wildcard instead of individual subcommands (pull was missing)
- echo, ls, grep, mkdir, mktemp, pwd for basic shell operations

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: tighten git to explicit subcommands, add missing shell utils

Restore explicit git subcommand list (diff, log, fetch, branch,
rev-parse, show, pull, checkout) instead of wildcard.
Add shell utilities (echo, ls, grep, mkdir, mktemp, pwd) that
Claude needs for skill script execution.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: full history checkout and explicit MCP context for review agent

Shallow clone (depth=1) prevented git diff/log/merge-base from working.
Agent couldn't read env vars for PR context due to tool permission
restrictions. Now fetches full history and instructs agent to use MCP
GitHub tools for PR metadata, passing context explicitly to sub-agents.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: bump review agent max-turns to 150

Previous 30-turn limit caused failures when agent spent turns on
permission errors. With those fixed, 150 gives ample room for
complex multi-file reviews with sub-agents.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* ci: reduce wasted turns in Claude Code review workflow

- Expose GH_TOKEN to MCP server for GitHub API authentication
- Inject PR context (number, branches, title) directly into prompt
- Disable MemCan tools unavailable in CI environment
- Add missing Bash patterns (git status/remote/merge-base) to allowedTools
- Remove overly broad Bash(bash *) wildcard from allowedTools
- Add MCP→gh CLI fallback guidance and no-chaining instruction

Saves ~8-13 API turns per review session and fixes MCP auth failures
that were preventing reviews from completing.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: probe GitHub MCP availability before review starts

Add explicit mcp__plugin_claudius_github__get_me probe as the first
action in the review prompt. The result (available/unavailable) is
propagated to all sub-agent prompts so they never waste turns
rediscovering MCP status.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* chore: minor tuning

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ste (#724)

- Remove MCP probe step — use MCP directly, fall back to gh CLI on failure
- Remove unconditional dash-platform skill load (saves ~1500 tokens/turn)
- Enforce Skill tool invocation for check-pr-comments and grumpy-review
  (Claude was skipping them and doing freestyle reviews)

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* fix: move list_core_wallets() RPC off the UI thread (#700)

Move synchronous list_core_wallets() RPC calls off the UI thread to
prevent UI freezes when Dash Core is slow or unreachable.

- Add CoreTask::ListCoreWallets backend task variant
- Add BackendTaskSuccessResult::CoreWalletsList result variant
- Convert AddNewWalletScreen constructor to async fetch pattern
- Convert ImportMnemonicScreen constructor to async fetch pattern
- Convert WalletsBalancesScreen::display_task_error to async fetch
- Add error recovery (retry on failure) and network-switch reset
- Remove TODO comments CMT-004, CMT-007, CMT-008

Closes #700

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: add manual test scenarios for async list_core_wallets (#700)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: clamp selected_core_wallet_index when core wallets list refreshes

Prevent potential out-of-bounds panic when display_task_result receives
a shorter wallet list than the currently selected index. Also corrects
the ACW-003 test scenario to match actual save behavior (auto-assigns
sole wallet rather than NULL).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: stop infinite retry loop and clear stale state on network switch

- SEC-001: Set core_wallets = Some(vec![]) on fetch error to break
  60fps retry loop in AddNewWalletScreen and ImportMnemonicScreen
- SEC-002: Clear pending_list_* state in WalletsBalancesScreen on
  network switch to prevent stale wallet hash from wrong network
- ACW-012/013: Correct test scenarios — modal screens on screen_stack
  don't receive change_context() from change_network()

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* ci: upload HTML review report as artifact

Instruct grumpy-review to render HTML instead of markdown and save
report.json + report.html to a known directory. Add upload-artifact
step so the interactive report (with Chart.js charts, severity
filtering, sorting) is downloadable from the Actions run.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: harden report artifact — pre-create dir, drop cp, explicit paths

Address bot review comments:
- Pre-create $REPORT_DIR before Claude runs (CodeRabbit)
- Remove Bash(cp *) from allowed tools; write directly to $REPORT_DIR (Copilot)
- Upload only report.json and report.html, not the whole directory (Copilot)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: simplify prompt — drop redundant directory note

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* fix: show user-friendly error for duplicate identity keys (#714)

Replace raw base64-encoded CBOR error with actionable messages when
adding a duplicate key to an identity. Match structured SDK error
variants (ConsensusError::StateError) instead of string parsing.

- Add TaskError variants: DuplicateIdentityPublicKey,
  DuplicateIdentityPublicKeyId, IdentityPublicKeyContractBoundsConflict
- Add broadcast_error_message() helper matching both SDK error paths
  (StateTransitionBroadcastError and Protocol/ConsensusError)
- 5 unit tests covering all variants + fallback
- Manual test scenarios document

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: correct error messages — duplicate keys are globally unique, not per-identity

DuplicatedIdentityPublicKeyStateError and DuplicatedIdentityPublicKeyIdStateError
are platform-wide uniqueness constraints, not per-identity. Updated error messages
to reflect that keys must be globally unique across all identities.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: remove incorrect "globally unique" claim from error messages

Only unique key types (ECDSA_SECP256K1, BLS12_381) require platform-wide
uniqueness. Non-unique types (ECDSA_HASH160, BIP13_SCRIPT_HASH,
EDDSA_25519_HASH160) can be shared across identities. Simplified messages
to avoid misleading users about key uniqueness rules.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* docs: consolidate manual test scenarios into single E2E file

Replace 10 per-feature test files (107 scenarios, 2224 lines) with a
single docs/ai-design/manual-tests.md containing 10 end-to-end
scenarios that each exercise multiple features. Update CLAUDE.md to
reference the consolidated file.

Deleted directories (test-only):
- 2025-02-25-spv-peer-rework
- 2026-02-24-address-nonce-column
- 2026-02-24-asset-lock-fee-fix
- 2026-02-24-dashmate-auto-update
- 2026-02-24-mine-blocks-dialog
- 2026-02-24-platform-sync-simplification
- 2026-02-24-spv-sync-error-status
- 2026-02-24-sync-status-panel
- 2026-03-03-fix-nonce-reset-on-refresh
- 2026-03-05-banner-details-overlap

Preserved directories (non-test design docs):
- 2026-02-16-spv-single-runtime-refactor
- 2026-02-17-unified-messages
- 2026-02-27-banner-review-fixes

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: add table of contents to manual-tests.md

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: add user personas for wallet redesign

Define three user archetypes (Everyday User, Power User, Platform
Developer) to guide progressive disclosure in the wallet UI. Add
concise README as index. Reference personas in CLAUDE.md documentation
section.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: add user stories catalog with 98 stories across 10 feature areas

Comprehensive user story inventory derived from source code, internal
docs, and published documentation. Each story tagged by persona
(Alex/Priya/Jordan), marked [Implemented] or [Gap], with acceptance
criteria. Covers wallet, send/receive, asset locks, identity, DPNS,
DashPay, tokens, contracts, developer tools, and network settings.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: add UX design patterns reference card

Concise UI/UX conventions covering design tokens, color system, buttons,
dialogs, forms, messages, tables, loading states, navigation, keyboard
accessibility, progressive disclosure, and responsive layout. All values
verified against theme.rs and component implementations.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: audit and update user stories, UX patterns, and CLAUDE.md

Full codebase audit of user-stories.md against implementation:
- Fix SND-005: [Gap] → [Implemented] (fee estimation exists)
- Fix NET-006: [Implemented] → [Gap] (no UI control)
- Add note to DPN-005 about Platform-level vote constraint
- Add 15 new stories: WAL-017..020, SND-006, IDN-012..013,
  DPY-009..011, TOK-015..017 (113 total, was 98)

Update ux-design-patterns.md with missing patterns:
- Password strength colors, interactive state colors
- Additional semantic color constants
- Shadow system tokens
- ComponentStyles button and input helpers
- Password input pattern (hold-to-reveal)

Add user stories catalog section to CLAUDE.md requiring
story updates on feature PRs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: remove hardcoded values from docs, refer to source files instead

Remove stale counts (scenario limits, story counts) and implementation
constants (pixel sizes, hex codes, padding values) that drift out of
sync with source code. Documents now explain usage/intent and point to
source files for exact values.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* chore: remove obsolete docs

* docs: note user-stories.md as exception to date-grouped docs rule

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* fix: show user-friendly error for duplicate identity keys (#714)

Replace raw base64-encoded CBOR error with actionable messages when
adding a duplicate key to an identity. Match structured SDK error
variants (ConsensusError::StateError) instead of string parsing.

- Add TaskError variants: DuplicateIdentityPublicKey,
  DuplicateIdentityPublicKeyId, IdentityPublicKeyContractBoundsConflict
- Add broadcast_error_message() helper matching both SDK error paths
  (StateTransitionBroadcastError and Protocol/ConsensusError)
- 5 unit tests covering all variants + fallback
- Manual test scenarios document

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: correct error messages — duplicate keys are globally unique, not per-identity

DuplicatedIdentityPublicKeyStateError and DuplicatedIdentityPublicKeyIdStateError
are platform-wide uniqueness constraints, not per-identity. Updated error messages
to reflect that keys must be globally unique across all identities.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: remove incorrect "globally unique" claim from error messages

Only unique key types (ECDSA_SECP256K1, BLS12_381) require platform-wide
uniqueness. Non-unique types (ECDSA_HASH160, BIP13_SCRIPT_HASH,
EDDSA_25519_HASH160) can be shared across identities. Simplified messages
to avoid misleading users about key uniqueness rules.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: return typed TaskError from broadcast_error and add_key_to_identity

- broadcast_error() now returns TaskError directly instead of String
- add_key_to_identity() returns Result<..., TaskError> — typed variants
  flow through without String round-trip
- run_identity_task() returns Result<..., TaskError> — other arms
  auto-convert via From<String>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Update src/backend_task/identity/add_key_to_identity.rs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* fix: use explicit Encoding::Base58 for Identifier::to_string() calls

The Platform SDK changed Identifier::to_string() to require an explicit
Encoding argument. Updated both production and test code to use
to_string(Encoding::Base58) instead of the no-arg variant.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: preserve typed SdkError in TaskError variants for duplicate-key errors (#731)

* Initial plan

* fix: log raw SdkError in broadcast_error to preserve technical context

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: embed raw SdkError as source_error field in typed TaskError variants

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: store Box<SdkError> as typed source in TaskError variants instead of debug string

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* style: fix rustfmt formatting issues in error.rs and add_key_to_identity.rs

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: address PR review comments for typed TaskError migration

- Convert standalone `broadcast_error()` fn to `impl From<SdkError> for TaskError`
  (per @lklimek review), moving logic and tests to `error.rs`
- Add `BroadcastError` variant with user-friendly Display message instead of
  leaking raw SDK text via `TaskError::Generic` (per CodeRabbit review)
- Change `refresh_identity`, `top_up_identity_from_platform_addresses`, and
  `transfer_to_addresses` to return `Result<_, TaskError>` instead of
  `Result<_, String>` (per CodeRabbit review)
- Simplify callers in `run_identity_task` — direct `.await` without
  unnecessary `Ok(self.foo().await?)` wrapping

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: replace BroadcastError with SdkError variant and user-friendly messages

Rename TaskError::BroadcastError to TaskError::SdkError and add
sdk_error_user_message() helper that inspects the SDK error variant to
produce actionable, user-friendly Display text for MessageBanner.

Covers: broadcast rejections, timeouts, stale nodes, DAPI client errors,
unavailable servers, cancellation, already-exists, nonce overflow/not-found.
Fallback includes the SDK's own Display text for unmatched variants.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: add error message tone and form guidelines to CLAUDE.md

Define rules for user-facing error text: audience (Everyday User persona),
structure (what happened + what to do), tone (calm, direct, no jargon),
and where technical details belong (details panel, not the message).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: refine error message guidelines — no support redirects, i18n-ready

- Remove "contact support" as an acceptable action — users must self-resolve
- Add Fluent i18n readiness rule: simple sentences, no fragment concatenation,
  placeholders only for dynamic values

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: make error messages i18n-ready and self-contained

- Remove "key hash" jargon from DuplicateIdentityPublicKeyId message
- Include platform/SDK error text as a Fluent-friendly { $error }
  placeholder in broadcast rejection and fallback messages
- Remove "review the details" references (not visible in basic mode)
- Never refer users to "contact support" or "details panel"
- Update CLAUDE.md error guidelines accordingly

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: refine error message guidelines — Base58 IDs allowed, prefer typed variants

- Clarify rule 4: "technical details" means raw error strings/codes/SDK
  internals, not user-meaningful identifiers; add exception note pointing
  to new rule 7.
- Add rule 7: Base58 IDs (contract, identity, document) are allowed in
  user-facing messages — they are opaque-but-copyable handles, not jargon.
- Add rule 8: prefer granular TaskError variants with #[source] over
  TaskError::Generic(format!(...)); Generic is a last resort.
- Fix "generic message" wording in Error banners section to avoid
  confusion with TaskError::Generic.
- Add soft guideline: consider moving repeated banner messages into
  TaskError variants for centralised wording and testability.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* refactor: address PR review comments — typed errors and clean user messages

- Add TaskError::IdentityNotFoundLocally variant with actionable Display
- Fix sdk_error_user_message: remove raw e.message from
  StateTransitionBroadcastError arm; use fixed, jargon-free string
- refresh_identity: bare ? on SDK fetch (From<SdkError> classifies);
  use IdentityNotFoundLocally; fix channel error to fixed string
- add_key_to_identity: bare ? on nonce/identity fetches; use
  IdentityNotFoundLocally for None identity; fixed string for
  IdentityUpdateTransition build failure; bare ? + Ok() on DB update
- identity/mod.rs: bare ? on top_up_from_addresses and
  transfer_credits_to_addresses (From<SdkError> handles classification)

Addresses review threads 11, 12, 13, 14, 16, 20, 21.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* fix: add message-based duplicate-key fallback and IdentitySaveError variant

- From<SdkError>: add .or_else() message-based fallback for
  StateTransitionBroadcastError with cause=None; if message contains
  "duplicate" (case-insensitive) map to DuplicateIdentityPublicKey —
  guards against #714 regression when DAPI omits structured cause
- Add regression test for cause=None message-only duplicate-key path
- Rename misleading test from_sdk_error_unknown_falls_back_to_broadcast_error
  → from_sdk_error_generic_variant_falls_back_to_sdk_error
- Add TaskError::IdentitySaveError { #[source] source: rusqlite::Error }
  with user-friendly Display; use at both update_local_qualified_identity
  callsites in top_up and transfer paths (threads 7, 8)
- Add TODO(i18n/ux) comment to sdk_error_user_message fallback arm (thread 17)

Addresses review threads 7, 8, 9, 17, 19.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* refactor: preserve source errors in typed TaskError variants

- Add IdentityUpdateTransitionError variant to preserve ProtocolError
  from try_from_identity_with_signer (was discarded via |_| Generic)
- Add InternalSendError variant to preserve SendError from channel send
  (was discarded via |_| Generic in refresh_identity)
- Wire IdentitySaveError in add_key_to_identity (bare ? produced
  TaskError::Sqlite with raw rusqlite text instead of friendly message)
- Add TODO for LockPoisoned variant in mod.rs wallet.read() path

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* refactor: use concrete SdkError type in error variants; drop dyn Error

- IdentityUpdateTransitionError.source_error: Box<dyn Error> → Box<SdkError>
  (ProtocolError converts via SdkError::Protocol(e))
- InternalSendError: struct variant → unit variant (SendError carries no
  diagnostic information worth preserving in the chain)
- CLAUDE.md rule 8: document source field type conventions — Box<SdkError>
  for SDK-originated errors, concrete domain type for non-SDK errors,
  omit #[source] when upstream carries no diagnostic value

Co-Authored-By: Claude Sonnet 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>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
* chore: rename "Developer mode" to "Expert mode" in UI strings

User-facing labels only — no code/variable renames.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* chore: shorten left panel expert label and add tooltips

Shorten "Expert Mode" to "Expert" in the left panel to fit the column
width, with a hover tooltip for context. Add tooltip to the network
chooser checkbox as well.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* chore: minor adjustments

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* Initial plan

* build: configure GitHub Copilot environment (copilot-setup-steps.yml)

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* build: narrow pre-build to tenderdash-proto and add pull_request trigger

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* build: align copilot-setup-steps with project workflow conventions

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* Update .github/workflows/copilot-setup-steps.yml

Co-authored-by: Copilot <175728472+Copilot@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>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* fix: tighten Claude review workflow permissions

- Add 10 missing shell script/Python permissions to --allowedTools
  (gh-resolve-review-threads, consolidate_reports, etc.)
- Add Bash(jq *) for JSON processing
- Remove Bash(cat *) and Bash(grep *) — agents use Read/Grep tools
- Document issues:write rationale (needed for gh pr edit --remove-label)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: remove Bash(python3 *) from review workflow allowedTools

All Python invocations are already covered by script-specific globs:
- Bash(*consolidate_reports.py *)
- Bash(*validate_report.py *)
- Bash(*generate_review_report.py *)

The only non-script usage was an agent improvising
`python3 -c "import shutil; shutil.copy2(...)"` instead of cp.
No need for a wide-open python3 permission.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* feat: add ResponseExt trait to enforce tooltip cursor policy

Introduce `ResponseExt` extension trait on `egui::Response` with three
methods — `info_tooltip` (Help cursor), `clickable_tooltip` (PointingHand),
and `disabled_tooltip` (NotAllowed) — that combine tooltip text with the
correct cursor icon in a single call.

Migrate all 56 `.on_hover_text()` callsites across 31 files to use the
new helpers, ensuring consistent cursor behavior project-wide. Update
UX design patterns doc to reference the new API.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address PR review comments on tooltip cursor policy

- Fix disabled_tooltip() to use on_disabled_hover_text() so tooltips
  actually appear on disabled widgets
- Split add_enabled() callsites to use clickable_tooltip when enabled
  and disabled_tooltip when disabled (4 files)
- Move info_tooltip from editable TextEdit fields to their labels to
  preserve the I-beam cursor on inputs (8 token screens)
- Use contextual tooltip on connection indicator (clickable only when
  Dash-Qt can be launched)
- Change locked security level selector from info_tooltip to
  disabled_tooltip
- Fix docs referencing StyledButton instead of ComponentStyles

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: make ResponseExt methods state-aware for chaining

clickable_tooltip now only applies when widget is enabled,
disabled_tooltip only when disabled. This enables clean chaining:

    ui.add_enabled(ready, button)
        .clickable_tooltip("Transfer")
        .disabled_tooltip("Fill fields first")
        .clicked()

Simplifies 4 callsites that previously needed if/else branching.
Adds comprehensive rustdocs with chaining contract for future
implementors.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address remaining PR review comments on tooltip policy

- Make info_tooltip truly state-agnostic by calling both on_hover_text
  and on_disabled_hover_text (fixes doc/impl mismatch)
- Change add_key_screen locked selector back to info_tooltip — the
  hover overlay from ui.interact() is always enabled, so state-aware
  disabled_tooltip was a silent no-op
- Chain disabled_tooltip on Actions button in identities_screen so
  inactive identities show explanation instead of click affordance

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: migrate remaining bare on_disabled_hover_text to disabled_tooltip

Addresses the last unresolved PR review comment: 4 callsites still
used bare on_disabled_hover_text() instead of the ResponseExt policy,
and the docs understated when disabled_tooltip is still needed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
)

* docs: promote i18n-ready strings to general coding rule (#660)

Move the i18n guideline out of the error-messages section into general
rules, broadening its scope to all user-facing strings (labels, tooltips,
messages). Remove direct Fluent reference, fix placeholder syntax to
Rust-style `{ name }`, and renumber remaining error rules.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: add typed error variants for migration (#660)

Add new TaskError variants (LockPoisoned, Database, CoreRpc, P2P),
new domain error types (P2PError, WalletError::KeyDerivation,
WalletError::Sighash, DashPayError::QueryCreation,
DashPayError::CryptoKeyParsing, DashPayError::PrivateKeyResolution,
ConfigError::SaveError), and replace the transparent Sqlite variant
with a user-friendly Database variant.

Drop Clone/PartialEq from DashPayError to support #[source] fields
and fix the two downstream callsites that depended on them.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: replace string errors with typed errors in spv/config/infra (#660)

- core_p2p_handler: eliminate ReadMessageError(String), use P2PError variants
  throughout internal methods; keep public API returning String for callers
  outside scope
- config: replace ConfigError::LoadError with SaveError in save() for all
  I/O operations (writeln, sync_all, persist)
- backend_task/mod: add map_err(TaskError::from) bridges for wallet task
  functions that still return String
- model/wallet: fix KeyDerivation source type mismatch (key_wallet::Error !=
  bip32::Error) by reverting to e.to_string() at derivation_path() callsites

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* refactor: replace string errors with typed TaskError in model/database/context (#660)

- Use WalletError::AddressError in utxos.rs for dashcore address errors
- Use TaskError::from(e) for RPC errors in broadcast_raw_transaction

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* refactor: replace string errors with typed TaskError in backend_task/ (#660)

Migrate callsites in src/backend_task/ from Result<T, String> to
Result<T, TaskError>:

- broadcast_state_transition: map SDK error via TaskError::from
- contract: direct ? for rusqlite, TaskError::from for SDK errors
- document: all 7 document operations migrated
- mod.rs: run_wallet_task return type updated to TaskError
- register_contract: sender error → InternalSendError, SDK → TaskError::from
- update_data_contract: sender error → InternalSendError, format strings → Generic
- wallet/*: lock poison → Generic, SDK errors → TaskError::from, ? for rusqlite

Build, clippy, and fmt all pass.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* refactor: replace string errors with typed TaskError in ui/ (#660)

Migrate rusqlite error callsites in src/ui/ to typed TaskError variants:

- `IdentityTokenInfo::try_from_*` functions in tokens_screen/structs.rs
  now return `Result<Self, TaskError>` — DB errors become
  `TaskError::Database` via `?` instead of `.map_err(|e| e.to_string())`.
  Callers use `MessageBanner::set_global` with the error which accepts
  `impl Display`, so no caller changes were needed.

- `save_private_info` in contact_profile_viewer.rs now returns
  `Result<(), TaskError>` — the rusqlite error is wrapped explicitly
  as `TaskError::Database { source: e }` for clear error semantics.

All other `map_err(|e| e.to_string())` callsites in src/ui/ were
reviewed and left unchanged — they are in UI-local validation functions
(clipboard, form validation, address generation) that do not feed into
the backend task error system.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix: remove useless TaskError::from conversions in wallet task dispatch (#660)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: fix all unresolved review comments — proof errors, asset lock, format! migrations (#740)

* Initial plan

* refactor: add typed TaskError variants, eliminate Generic from backend_task files

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: eliminate Result<T,String> from context/transaction_processing, add typed TaskError variants

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: migrate context/wallet_lifecycle.rs from Result<T,String> to Result<T,TaskError>

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: add source_error to ProofError, add log_drive_proof_error helper, migrate token tasks to TaskError

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: replace TaskError::Internal with granular variants, migrate identity tasks to TaskError

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* style: apply cargo +nightly fmt to fix CI formatting failures

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: fix all unresolved review comments (Claude + lklimek)

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: remove TaskError::Generic and TaskError::Other, replace with specialized variants (#744)

* Initial plan

* refactor: remove TaskError::Generic and TaskError::Other, add specialized variants

- Remove Generic(String) and Other(Box<dyn Error>) variants from TaskError
- Add SerializationError, IdentifierParsingError, IdentityCreationError,
  InvalidPrivateKey, DpnsFetchError, AssetLockNotValidForWallet,
  AssetLockInstantLockExpiredNotChainlocked, PlatformFetchError variants
- Update From<String> bridge to map to UserInput instead of Generic
- Replace TaskError::Generic("Identity not found") with IdentityNotFoundLocally
- Replace TaskError::Generic("Contract not found") with DataContractNotFound
- Replace TaskError::Generic(e.to_string()) for bincode with SerializationError
- Fix instant-lock fallback to use Debug repr instead of Display for string matching
- Fix poisoned lock spinning in transaction_processing.rs
- Fix silently dropped wallet locks in load_identity.rs and load_identity_by_dpns_name.rs
- Change contract_token_db::remove_wallet to return TaskError instead of String
- Use typed variants in register_identity.rs and top_up_identity.rs
- Use ? operator for lock errors in core/mod.rs

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: address remaining PR 739 review comments

- Fix wallet removal atomicity: acquire write lock before DB removal
- Fix proof-log write failures: warn instead of silently dropping
- Fix domain jargon in AssetLock* error messages (no "asset lock", "instant lock")
- Fix token metadata update atomicity: use upsert instead of remove+insert

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: address code review comments - improve error messages and add TODO notes

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: replace UserInput catch-all with granular DashCoreStartError, OperationNotAvailableOnNetwork, and LegacyError variants

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: improve DashCoreStartError message and simplify OperationNotAvailableOnNetwork display

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: address PR #739 review comments — structural error matching, lock poisoning cleanup, parameterized RequestType

- Parameterize `log_drive_proof_error` with `RequestType` instead of hardcoding `BroadcastStateTransition` (26 callsites across 15 files)
- Replace all `map_err` to `LockPoisoned` with `?` operator using blanket `From<PoisonError<T>>` impl; add doc comment explaining the intentional design
- Change `CreditCalculationOverflow` variant to carry raw data fields (`amount`, `credits_per_duff`) instead of pre-formatted text
- Add `TaskError::AssetLockInstantLockProofInvalid` variant with structural SDK error matching, replacing fragile `format!("{:?}").contains(...)` in register_identity and top_up_identity
- Replace `StaleNode` string matching with structural `Error::StaleNode(_)` match in contested names queries
- Add `is_unique_constraint_violation()` helper for SQLite error matching in import_mnemonic_screen
- Fix wrong error variant: `DocumentNotFound` → `DataContractNotFound` in update_token_config
- Add TODO for `BroadcastError(String)` pending `ScreenLike::display_message` trait redesign

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: address remaining PR #739 review comments — lock poisoning, error interpolation, StaleNode matching

- Remove all remaining explicit `map_err` to `LockPoisoned` across 10 files, using `?` with blanket `From<PoisonError<T>>` impl
- Fix `context/mod.rs` sites returning `Result<_, String>` to use `TaskError::from(e).to_string()` instead of discarding resource info
- Add `StaleNode` structural matching to `query_dpns_vote_contenders.rs` for consistency with `query_dpns_contested_resources.rs`
- Interpolate `operation` and `allowed_networks` fields in `OperationNotAvailableOnNetwork` error message
- Narrow `is_unique_constraint_violation` to check `extended_code: 2067` (SQLITE_CONSTRAINT_UNIQUE)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: address final PR #739 review comments — jargon removal, dual signal fix

- Replace protocol jargon ("asset lock", "instant lock proof",
  "chain lock proof") with user-friendly language in three
  TaskError Display messages per CLAUDE.md error guidelines
- Fix dual success+error signal in register_contract.rs: return
  Ok(ContractSavedAfterProofError) instead of sending success
  via channel then returning Err(ProofError)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: add 6 new TaskError variants for LegacyError removal

Add PlatformInfoFetchError, EncryptionError, WalletDatabasePersistError,
AvatarProcessingError, MasterKeyNotFound, and TokenQueryError variants
to prepare for removing From<String> and LegacyError.

Also fix test assertion for updated jargon-free error message.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: migrate system/grovestark/mnlist/platform_info to TaskError

Replace Result<_, String> with Result<_, TaskError> in four domain
dispatcher modules, eliminating reliance on From<String> for TaskError
in these paths.

- system_task: use ? on rusqlite errors (via #[from])
- grovestark: use ? on GroveSTARKError (via #[from])
- mnlist: replace .unwrap() on RwLock with proper error handling
- platform_info: wrap SDK errors in TaskError::SdkError, use
  TaskError::Generic for document property extraction errors

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: remove From<String> for TaskError and LegacyError variant

Remove the `impl From<String> for TaskError` bridge and `TaskError::LegacyError(String)`
variant. All 45 files that relied on string-to-TaskError conversion now use typed variants.

Migration patterns applied:
- RwLock/Mutex `.map_err(|e| e.to_string())?` → bare `?` (From<PoisonError>)
- rusqlite `.map_err(|e| e.to_string())?` → bare `?` (Database #[from])
- dashcore_rpc errors → bare `?` (From<dashcore_rpc::Error>)
- SDK errors → bare `?` (From<SdkError>)
- String literals → typed variants (UserInput, WalletNotFound, etc.)
- Model/SPV boundary → .map_err(TaskError::UserInput) at callsite

New variants used: PlatformInfoFetchError, EncryptionError,
WalletDatabasePersistError, AvatarProcessingError, MasterKeyNotFound,
TokenQueryError.

45 files changed, 859 insertions(+), 1000 deletions(-)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: address 16 review findings across error handling and conventions

Error design (error.rs):
- Remove dead AvatarProcessingError variant (CODE-004)
- Add ContractSchemaMismatch variant for contested index (CODE-006)
- Add WithdrawalDocumentParsingError variant for withdrawal parsing (CODE-007)
- Route 9 platform_info.rs callsites through From<SdkError> classification (CODE-001)

Correctness fixes:
- Fix dual signal in update_data_contract: return Ok instead of send+Err (CODE-002)
- Return Ok(None) instead of Err(QueryReturnedNoRows) in contract_token_db (CODE-003)

P2P module:
- Public API returns P2PError instead of String, enabling #[from] conversion (CODE-008)
- Add ProtocolError variant for protocol violations instead of synthetic io::Error (CODE-009)

Error message quality:
- Rewrite ~20 DashPayError Display messages for Everyday User audience (PROJ-001)
- Rewrite ConfigError::NoValidConfigs and WalletError::AddressError Display (PROJ-004)
- Update CLAUDE.md rule 7: remove stale Generic references (PROJ-002)
- Clarify i18n placeholder style in CLAUDE.md (PROJ-005)

Mechanical improvements:
- Replace explicit TaskError::Database{source:e} with ? via #[from] (CODE-010)
- Replace ok_or_else with ok_or for unit-like TaskError variants (CODE-011)
- Replace .unwrap() with .unwrap_or_else(|e| e.into_inner()) in identity_db (CODE-017)
- Simplify lock poisoning double-conversion in context/mod.rs (CODE-018)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: replace `UserInput(String)` with typed `TaskError` variants (#751)

* Initial plan

* refactor: replace all UserInput usages with typed TaskError variants

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: improve error message clarity (reviewer feedback)

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: merge RPC/context variants, split NotTokenDistributionRecipient, use #[source] for typed errors

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: address review feedback on TaskError migration — jargon, transparent errors, variant cleanup (#753)

* Initial plan

* refactor: address all review feedback — transparent errors, IdentitySaveError, DpnsFetchError, CLAUDE.md

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* fix: resolve CI formatting failures (#754)

* Initial plan

* fix: run cargo +nightly fmt --all to fix CI formatting failures

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* fix: address 3 PR review findings — error chain, transitional note, vote errors

- DashPayError::SdkError: preserve error chain with #[source] Box<dash_sdk::Error>
  instead of stringifying to reason: String
- CLAUDE.md rule 7: add transitional note acknowledging detail: String pattern
- vote_on_dpns_name: route SDK errors through TaskError for user-friendly messages

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address review round 4 — typed errors, dead code, lock safety

- vote_on_dpns_name: use ContractSchemaMismatch instead of fake SdkError::Generic
- core/mod.rs: migrate 7 payment functions from Result<_, String> to Result<_, TaskError>
- load_identity_from_wallet: replace .unwrap() with ? on wallet RwLock operations
- Remove dead UiError type (src/ui/error.rs) — zero usages

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: strengthen error variant policy — never use String fields for user messages

CLAUDE.md rule 7: replace transitional note with strict rule — error variants
must not contain String fields that hold user-facing messages. Use #[source]
with typed errors, or fieldless variants. Replace existing String-field variants
when encountered.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
The loading state was being cleared by display_message() and at the
top of display_task_result() for ALL result variants. When unrelated
backend tasks completed while a profile search was in progress, they
would clear the loading flag, causing 'No users found' to flash
briefly before actual results arrived.

Fix: only clear loading state when we receive our specific
DashPayProfileSearchResults variant or on task error. Unrelated
task completions no longer interfere with the search spinner.

Closes #684

Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com>
Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
* chore: use claudius-review-action for PR reviews

Replace inline 119-line review workflow with single-step call to
lklimek/claudius-review-action composite action. Points at
feat/composite-action branch for testing — will pin to v1 tag
once validated.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* chore: tweaking

* chore: remove not working claude workflow

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* ci: fix model config and add MemCan to review workflow

Move model selection from invalid `claude_model` input to
`ANTHROPIC_MODEL` env var. Add `memcan_url` and `memcan_api_key`
inputs for persistent memory support.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ci: use repository variable for MemCan URL

MEMCAN_URL is not sensitive — use vars instead of secrets.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…rged

# Conflicts:
#	.github/workflows/clippy.yml
#	.github/workflows/tests.yml
#	CLAUDE.md
#	Cargo.lock
#	Cargo.toml
#	src/app.rs
#	src/backend_task/core/mod.rs
#	src/backend_task/error.rs
#	src/backend_task/mod.rs
#	src/context/mod.rs
#	src/database/initialization.rs
#	src/spv/manager.rs
#	src/ui/components/top_panel.rs
#	src/ui/components/wallet_unlock.rs
#	src/ui/contracts_documents/add_contracts_screen.rs
#	src/ui/contracts_documents/document_action_screen.rs
#	src/ui/contracts_documents/group_actions_screen.rs
#	src/ui/contracts_documents/register_contract_screen.rs
#	src/ui/contracts_documents/update_contract_screen.rs
#	src/ui/dashpay/contact_requests.rs
#	src/ui/dashpay/profile_screen.rs
#	src/ui/dashpay/profile_search.rs
#	src/ui/identities/add_new_identity_screen/by_wallet_qr_code.rs
#	src/ui/identities/identities_screen.rs
#	src/ui/identities/keys/key_info_screen.rs
#	src/ui/mod.rs
#	src/ui/tokens/destroy_frozen_funds_screen.rs
#	src/ui/tokens/direct_token_purchase_screen.rs
#	src/ui/tokens/freeze_tokens_screen.rs
#	src/ui/tokens/mint_tokens_screen.rs
#	src/ui/tokens/pause_tokens_screen.rs
#	src/ui/tokens/resume_tokens_screen.rs
#	src/ui/tokens/set_token_price_screen.rs
#	src/ui/tokens/tokens_screen/my_tokens.rs
#	src/ui/tokens/unfreeze_tokens_screen.rs
#	src/ui/tokens/update_token_config.rs
#	src/ui/tools/masternode_list_diff_screen.rs
#	src/ui/wallets/asset_lock_detail_screen.rs
#	src/ui/wallets/create_asset_lock_screen.rs
#	src/ui/wallets/single_key_send_screen.rs
#	src/ui/wallets/wallets_screen/mod.rs
…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>
Merge origin/zk branch which adapts to dashpay/platform v3.1-dev API:
- OrchardProver trait wrapper (CachedProver) replaces raw ProvingKey refs
- OrchardAddress::from_raw_bytes now returns Result
- FeeLevel removed; use FeeRate::normal() directly
- WalletManager.create_unsigned_payment_transaction removed; use TransactionBuilder
- SyncState::Initializing removed; replaced with WaitForEvents
- SpvClient ownership model changed (no longer Arc-wrapped)

Resolved conflicts preserving typed TaskError variants from our branch
while adopting origin/zk's v3.1-dev API adaptations. Fixed branch name
typo (3.1-dev → v3.1-dev) in Cargo.toml.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏆 Claudius Code Review — PR #644

Verdict: APPROVED

A well-executed backport merge that integrates 8 reviewed PRs from v1.0-dev into zk. The Grand Admiral is... grudgingly impressed.

What went right

  • Merge conflict resolution: All 8 conflict files resolved cleanly — zero residual markers. The wallets screen elegantly combines v1.0-dev sync status with ZK shielded tab features.
  • New Secret type: Exemplary security component — mlock, full-capacity zeroize, timing-resistant equality, redacted Debug. This is what security-sensitive code should look like.
  • TaskError enum: Well-designed typed error envelope with thorough SDK error classification and 13 focused tests.
  • PR documentation: Transparent conflict resolution documentation, constituent PR tracking, SDK dependency rationale. Other PRs should aspire to this level of clarity.

Summary Statistics

Severity Count
CRITICAL 0
HIGH 0
MEDIUM 9
LOW 21
INFO 1

Key MEDIUM findings (see inline comments)

Most are pre-existing patterns this PR inherits, not introduces:

  • SEC-005: .env file permissions should be 0600
  • PROJ-001/003: CI uses deprecated actions-rs and bypasses safe-cargo.sh
  • RUST-016: Float arithmetic for financial fee estimation
  • RUST-006/008: Dialog overlay ordering inconsistency
  • RUST-008: ConfirmationDialog is_open lifecycle bug
  • RUST-010: unwrap() on RwLock::write() in wallet unlock UI
  • RUST-009: Inconsistent error variant for address network mismatch

None are blocking. All suitable for follow-up issues.

📊 Full report available as CI artifact (report.html).

🤖 Co-authored by Claudius the Magnificent AI Agent"
📊 View full HTML review report

Comment thread src/config.rs
ConfigError::LoadError(format!("Failed to persist temp config file: {}", e))
})?;
env_file
.persist(&env_file_path)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SEC-005] MEDIUM — .env file with RPC credentials has default OS permissions\n\nThe .env file containing core_rpc_password (and potentially wallet_private_key) is written here via persist() with default OS permissions — typically 0644 (world-readable) on Linux.\n\nOn shared systems, any local user or process can read these credentials.\n\nSuggestion: After persist(), restrict file permissions:\nrust\n#[cfg(unix)]\n{\n use std::os::unix::fs::PermissionsExt;\n std::fs::set_permissions(&env_file_path, std::fs::Permissions::from_mode(0o600))\n .unwrap_or_else(|e| tracing::warn!(\"Could not restrict .env permissions: {e}\"));\n}\n\n\nPre-existing issue, not introduced by this PR — track as follow-up.\n\n🤖 Claudius the Magnificent · SEC-005"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[RUST-008] MEDIUM — ConfirmationDialog doesn't set is_open=false after button click\n\nWhen the user clicks Confirm or Cancel, self.status is set but self.is_open stays true — it's only set to false when the window's X button is clicked (line 257). This means the dialog will re-render on the next frame after a button click.\n\nSelectionDialog handles this correctly at line 309-311:\nrust\nif final_response.is_some() {\n self.is_open = false;\n}\n\n\nSuggestion: Add the same pattern after line 260.\n\n---\n\n**[RUST-006] MEDIUM — Overlay uses Order::Background instead of Order::Middle\n\nThe dark scrim overlay at line 149 uses Order::Background, which may paint behind normal UI content rather than above it. SelectionDialog correctly uses Order::Middle. Same issue in InfoPopup at line 62.\n\nSuggestion:** Change both to Order::Middle for correct overlay behavior.\n\n🤖 Claudius the Magnificent · RUST-006, RUST-008"

@@ -51,7 +45,6 @@ pub trait ScreenWithWalletUnlock {
if let Some(wallet_guard) = self.selected_wallet_ref().clone() {
let mut wallet = wallet_guard.write().unwrap();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[RUST-010] MEDIUM — unwrap() on RwLock::write() will panic on poisoned lock\n\nBoth wallet_unlock.rs (here, line 16, and line 46) and wallet_unlock_popup.rs (line 175) use .write().unwrap() on the wallet RwLock. If any thread previously panicked while holding this lock, the UI will also panic during a user-initiated unlock attempt.\n\nThe read path already handles this gracefully with .ok() — the write path should be equally defensive.\n\nSuggestion:\nrust\nlet Ok(mut wallet) = wallet_guard.write() else {\n self.error_message = Some(\n \"Could not access wallet data. Please restart the application.\".into()\n );\n return false;\n};\n\n\n🤖 Claudius the Magnificent · RUST-010"

})?;

let total_amount: u64 = recipients.iter().map(|(_, amt)| *amt).sum();
let mut scale_factor = 1.0f64;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[RUST-016] MEDIUM — Fee estimation uses floating-point scaling for financial amounts\n\nbuild_spv_unsigned_transaction_multi uses f64 scale_factor to adjust payment amounts when subtract_fee_from_amount is true. The cast (*amt as f64 * scale_factor) as u64 (line 696) can silently truncate, and the convergence check uses a relative tolerance (0.0001) rather than an absolute duff-based threshold.\n\nFor a financial application handling real funds, integer arithmetic would be safer:\nrust\n// Instead of float scaling:\nlet reduced = (amount * (total - fee)) / total;\n\n\nPre-existing pattern, not introduced by this PR — but worth tracking for a future hardening pass.\n\n🤖 Claudius the Magnificent · RUST-016"

addr.network(),
self.network
));
return Err(TaskError::WalletPaymentFailed {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[RUST-009] MEDIUM — Address network mismatch uses wrong error variant in HD wallet path\n\nThe HD wallet path here returns WalletPaymentFailed with a detail string for network mismatch. However, the single-key wallet path (send_single_key_wallet_payment.rs:43) correctly uses the dedicated TaskError::AddressNetworkMismatch variant.\n\nThis inconsistency means the same user action produces different error types depending on the wallet type.\n\nSuggestion: Use TaskError::AddressNetworkMismatch here too, or use .require_network() as the single-key path does.\n\n🤖 Claudius the Magnificent · RUST-009"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[PROJ-001 + PROJ-003] MEDIUM — CI uses deprecated actions-rs and doesn't use safe-cargo.sh\n\nTwo related CI issues:\n\n1. Deprecated actions: actions-rs/toolchain@v1, actions-rs/cargo@v1, and actions-rs/clippy-check@v1 are from an archived, unmaintained GitHub org. The macOS release job already uses dtolnay/rust-toolchain as replacement.\n\n2. Supply chain risk: CLAUDE.md mandates scripts/safe-cargo.sh for CI to prevent build scripts from accessing secrets (GITHUB_TOKEN, etc.), but workflows use actions-rs/cargo directly, bypassing this protection.\n\nSuggestion: Replace actions-rs/* with dtolnay/rust-toolchain@stable + run: scripts/safe-cargo.sh <command>. Same applies to clippy.yml and copilot-setup-steps.yml.\n\nPre-existing — track as a follow-up CI hardening issue.\n\n🤖 Claudius the Magnificent · PROJ-001, PROJ-003"

@QuantumExplorer QuantumExplorer merged commit 62961b1 into zk Mar 22, 2026
7 of 8 checks passed
@QuantumExplorer QuantumExplorer deleted the zk-extract/all-merged branch March 22, 2026 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

human review This PR is ready for human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants