chore: merge v1.0-dev into zk and consolidate migrations

[lklimek]

Summary

• Consolidates DB migrations v28–v32 into a single idempotent v33 step
• Prevents version-collision issues when users switch between v1.0-dev, zk, and zk-fixes branches (all used v28 for different migrations)
• All sub-migrations use IF NOT EXISTS / column-existence checks, safe to re-run on databases that already applied some or all steps

Migrations included in v33

1. add_core_wallet_name_column — adds core_wallet_name to wallets
2. init_contacts_tables — creates contact request tables
3. create_shielded_tables — creates shielded_notes table
4. create_shielded_wallet_meta_table — creates shielded_wallet_meta table
5. rename_network_dash_to_mainnet — renames "dash" network to "mainnet"
6. add_wallet_transaction_status_column — adds status to wallet_transactions
7. add_nullifier_sync_timestamp_column — adds last_nullifier_sync_timestamp to shielded_wallet_meta

Companion PRs: #788 (v1.0-dev backport), same commit on zk-fixes.

Test plan

• Fresh install: create_tables() sets version to 33, all tables exist
• Upgrade from v27 (pre-fork): v28–v32 are no-ops, v33 applies all migrations
• Upgrade from v32 (existing zk user): v33 re-applies idempotently, no errors

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[coderabbitai]

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: 7f72cd4f-40a0-44db-b302-49c830bf0b69

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 fix/consolidate-migrations-v33-zk

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[thepastaclaw]

Code Review

The merge of v1.0-dev into the zk branch introduces duplicate method definitions for Database that would cause a compile error — create_shielded_tables, create_shielded_wallet_meta_table, and add_nullifier_sync_timestamp_column are defined in both initialization.rs and the new shielded.rs. Beyond this critical merge issue, the migration consolidation design is sound with correct idempotency guards. Several agent findings about missing shielded table coverage turned out to be false positives.

Reviewed commit: 9d4d65a

🔴 1 blocking | 🟡 4 suggestion(s) | 💬 3 nitpick(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/database/initialization.rs`:
- [BLOCKING] lines 941-997: Duplicate method definitions for Database will cause compile error
  The merge of v1.0-dev brought `create_shielded_tables` (line 941), `create_shielded_wallet_meta_table` (line 968), and `add_nullifier_sync_timestamp_column` (line 984) into `initialization.rs` as private methods. However, `shielded.rs` (new in this PR) already defines identical `pub(crate)` methods on `impl Database`. Rust does not allow duplicate method names across `impl` blocks for the same type, so this will not compile. Remove the three duplicate private methods from `initialization.rs` — the v33 migration at line 65 will resolve to the `pub(crate)` versions in `shielded.rs`.
- [SUGGESTION] line 933: Schema DEFAULT for status column differs between migration and fresh install
  The migration adds `status INTEGER NOT NULL DEFAULT 2` (Confirmed) to existing rows, which is correct. But the fresh `CREATE TABLE` in `wallet.rs:391` uses `DEFAULT 0` (Unconfirmed). The migration default persists in the column schema, so any future INSERT that omits `status` on a migrated database would get Confirmed (2) instead of Unconfirmed (0). This is harmless if all INSERT paths always specify `status` explicitly, but it's a latent inconsistency worth a brief comment.
- [SUGGESTION] lines 58-70: No test exercises the v33 consolidated migration path
  The consolidated v33 migration is the most complex migration in the codebase (7 idempotent sub-steps covering multiple branch-collision scenarios), but no test starts from a v27/v28/v31 database shape and verifies the final schema. Given the branch-collision motivation, a regression test would add confidence.

In `src/model/wallet/mod.rs`:
- [SUGGESTION] lines 757-763: spv_confirmed_balance returns None for a genuine zero-balance wallet
  When all three balance fields (total, confirmed, unconfirmed) are 0, the function returns `None` (not-yet-synced) instead of `Some(0)`. This makes it impossible for callers to distinguish a synced zero-balance wallet from an unsynced one. Test waiters that retry on `None` would loop forever on a funded-then-drained wallet. Consider tracking a `has_synced` flag or similar sentinel.

In `src/backend_task/error.rs`:
- [SUGGESTION] lines 608-609: WalletKeyDerivationFailed uses String field instead of typed #[source]
  Per project conventions in CLAUDE.md, error variants should use typed `#[source]` fields rather than `String` fields. `WalletKeyDerivationFailed { detail: String }` stores the stringified BIP32 error. Consider wrapping the upstream error type with `#[source]` instead.

[thepastaclaw]

💬 Nitpick: Doc comment for migration 29 (rename) is attached to the migration 30 function

The doc comment block at lines 917–924 describes both migration 29 (rename network) and migration 30 (add status column), but it's attached to add_wallet_transaction_status_column. The actual rename_network_dash_to_mainnet function at line 999 has no doc comment. Move the migration 29 doc comment to rename_network_dash_to_mainnet.

_{source: ['claude']}

[thepastaclaw]

🟡 Suggestion: Schema DEFAULT for status column differs between migration and fresh install

The migration adds status INTEGER NOT NULL DEFAULT 2 (Confirmed) to existing rows, which is correct. But the fresh CREATE TABLE in wallet.rs:391 uses DEFAULT 0 (Unconfirmed). The migration default persists in the column schema, so any future INSERT that omits status on a migrated database would get Confirmed (2) instead of Unconfirmed (0). This is harmless if all INSERT paths always specify status explicitly, but it's a latent inconsistency worth a brief comment.

_{source: ['claude']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/database/initialization.rs`:
- [SUGGESTION] line 933: Schema DEFAULT for status column differs between migration and fresh install
  The migration adds `status INTEGER NOT NULL DEFAULT 2` (Confirmed) to existing rows, which is correct. But the fresh `CREATE TABLE` in `wallet.rs:391` uses `DEFAULT 0` (Unconfirmed). The migration default persists in the column schema, so any future INSERT that omits `status` on a migrated database would get Confirmed (2) instead of Unconfirmed (0). This is harmless if all INSERT paths always specify `status` explicitly, but it's a latent inconsistency worth a brief comment.

[thepastaclaw]

🟡 Suggestion: spv_confirmed_balance returns None for a genuine zero-balance wallet

When all three balance fields (total, confirmed, unconfirmed) are 0, the function returns None (not-yet-synced) instead of Some(0). This makes it impossible for callers to distinguish a synced zero-balance wallet from an unsynced one. Test waiters that retry on None would loop forever on a funded-then-drained wallet. Consider tracking a has_synced flag or similar sentinel.

_{source: ['claude']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/model/wallet/mod.rs`:
- [SUGGESTION] lines 757-763: spv_confirmed_balance returns None for a genuine zero-balance wallet
  When all three balance fields (total, confirmed, unconfirmed) are 0, the function returns `None` (not-yet-synced) instead of `Some(0)`. This makes it impossible for callers to distinguish a synced zero-balance wallet from an unsynced one. Test waiters that retry on `None` would loop forever on a funded-then-drained wallet. Consider tracking a `has_synced` flag or similar sentinel.

[thepastaclaw]

🟡 Suggestion: WalletKeyDerivationFailed uses String field instead of typed #[source]

Per project conventions in CLAUDE.md, error variants should use typed #[source] fields rather than String fields. WalletKeyDerivationFailed { detail: String } stores the stringified BIP32 error. Consider wrapping the upstream error type with #[source] instead.

_{source: ['claude']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/backend_task/error.rs`:
- [SUGGESTION] lines 608-609: WalletKeyDerivationFailed uses String field instead of typed #[source]
  Per project conventions in CLAUDE.md, error variants should use typed `#[source]` fields rather than `String` fields. `WalletKeyDerivationFailed { detail: String }` stores the stringified BIP32 error. Consider wrapping the upstream error type with `#[source]` instead.

[thepastaclaw]

💬 Nitpick: rename_network_dash_to_mainnet wraps errors in semantically wrong rusqlite variant

map_err converts the real error into rusqlite::Error::InvalidParameterName, which is semantically unrelated to a migration UPDATE failure. This is a code smell — consider a more appropriate error variant or propagating the original error with additional context.

_{source: ['claude']}

[thepastaclaw]

💬 Nitpick: TransactionStatus::from_u8 silently maps unknown values to Unconfirmed

Unknown u8 values (≥4) silently become Unconfirmed with no diagnostic. In a financial application, a debug! or warn! log for unexpected values would help catch schema evolution mismatches early.

_{source: ['claude']}

[thepastaclaw]

🟡 Suggestion: No test exercises the v33 consolidated migration path

The consolidated v33 migration is the most complex migration in the codebase (7 idempotent sub-steps covering multiple branch-collision scenarios), but no test starts from a v27/v28/v31 database shape and verifies the final schema. Given the branch-collision motivation, a regression test would add confidence.

_{source: ['claude', 'codex']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/database/initialization.rs`:
- [SUGGESTION] lines 58-70: No test exercises the v33 consolidated migration path
  The consolidated v33 migration is the most complex migration in the codebase (7 idempotent sub-steps covering multiple branch-collision scenarios), but no test starts from a v27/v28/v31 database shape and verifies the final schema. Given the branch-collision motivation, a regression test would add confidence.

[thepastaclaw]

PR #789 review thread resolution status

lklimek asked me to check which of my review comments here are already fixed in PR #786 and resolve them. I've done the cross-check (see PR #786 comment), but I don't have permission to resolve review threads on this PR.

Already fixed in PR #786 (please resolve these threads):

1. 🔴 Duplicate method definitions (initialization.rs) — Already resolved ✅
2. 💬 Doc comment misplacement (initialization.rs) — Moot since the methods were removed in feat: AddressInput component, RPC error handling, shielded retry/sync improvements #786

Not applicable to PR #786 (only exist in #789's code):
3. spv_confirmed_balance zero-balance ambiguity (wallet/mod.rs)
4. WalletKeyDerivationFailed using String instead of typed #[source] (error.rs)
5. TransactionStatus::from_u8 silent fallback (wallet/mod.rs)

Still present in PR #786 (suggestions, non-blocking):
6. Schema DEFAULT mismatch for status column (initialization.rs)
7. rename_network_dash_to_mainnet error wrapping (initialization.rs)
8. No test for v33 migration path (initialization.rs)

@lklimek could you resolve threads #1 and #2? I don't have the permission to do so.

[thepastaclaw]

Code Review

Trivial incremental addressing the blocking duplicate-method finding. The 3 duplicate shielded helper methods (create_shielded_tables, create_shielded_wallet_meta_table, add_nullifier_sync_timestamp_column) removed from initialization.rs — they already exist in shielded.rs. Doc comments reorganized to correct methods. Inline comment added explaining migration DEFAULT 2 vs fresh install DEFAULT 0.

Reviewed commit: 0d4e359

[QuantumExplorer]

Approved, but tests need to pass