fix(db): consolidate migrations v28-v32 into v33#788
Conversation
Resolves version numbering collision between zk and v1.0-dev branches: the zk branch used v28 for shielded tables while v1.0-dev used v28 for contacts. After merging, users migrating from either branch could end up with missing tables depending on which version their DB was at. v33 runs all sub-migrations idempotently in one step, ensuring all tables exist regardless of prior migration history. Shielded pool tables are created but unused on v1.0-dev — they prepare the schema for the upcoming zk merge. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDatabase schema version bumped from 29 to 33; migrations for 28–32 are effectively consolidated as no-ops, while version 33 applies grouped idempotent sub-migrations (wallet name, contacts, shielded pool/tables, wallet metadata, nullifier sync timestamp, network rename, transaction status). New init path creates shielded tables. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Consolidates the previously separate DB migrations v28–v32 into a single v33 migration to avoid schema-version collisions across branches, while also introducing shielded-pool tables needed for upcoming ZK features.
Changes:
- Bumps
DEFAULT_DB_VERSIONto 33 and makes versions 28–32 no-op placeholders. - Adds a consolidated v33 migration that applies the combined v28–v32 schema updates in an idempotent way.
- Adds shielded table creation during fresh database initialization.
Comments suppressed due to low confidence (1)
src/database/initialization.rs:1017
rename_network_dash_to_mainnetclaims to update every table that storesnetwork, but the new shielded tables (shielded_notes,shielded_wallet_meta) also have anetworkcolumn and are not included in the update list. If a user migrates from a branch where these tables already exist withnetwork='dash', those rows will remain inconsistent. Add the shielded tables to thetableslist (or otherwise ensure they’re updated too).
let tables = [
"settings",
"wallet",
"identity_token_balances",
"platform_address_balances",
"utxos",
"asset_lock_transaction",
"identity",
"contested_name",
"contestant",
"contract",
"scheduled_votes",
"dashpay_profiles",
"dashpay_contact_requests",
"dashpay_contacts",
"wallet_transactions",
"single_key_wallet",
"token",
];
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/database/initialization.rs`:
- Around line 54-57: The migration loop currently advances and persists
database_version for the placeholder range 28..=32 (the match arm "28..=32 =>
{}"), which can leave the DB at version 32 if the consolidated v33 migration
fails; modify try_perform_migration() so that these placeholder versions are not
committed individually—either wrap the entire 28..=33 work in a single
transaction or change the 28..=32 path to be a no-op that does not call the code
that persists/commits database_version, and only write/update database_version =
33 after the v33 migration completes successfully (i.e., ensure the
commit/update to database_version happens once, post-success, not for each
placeholder version).
- Around line 966-977: The initial table creation in
create_shielded_wallet_meta_table is missing the last_nullifier_sync_timestamp
column so fresh installs start at v33 without it; update the SQL in
create_shielded_wallet_meta_table to include "last_nullifier_sync_timestamp
INTEGER NOT NULL DEFAULT 0" (alongside last_nullifier_sync_height) so the fresh
schema matches the later migrations and retains the existing PRIMARY KEY
(wallet_seed_hash, network).
- Around line 58-69: The migration branch that calls
rename_network_dash_to_mainnet() currently updates only some tables and neglects
shielded tables; update rename_network_dash_to_mainnet to also rewrite
network='dash' → 'mainnet' for the shielded tables by adding UPDATE statements
for the shielded_notes and shielded_wallet_meta tables (use the same
transaction/tx path and error handling as other updates in
rename_network_dash_to_mainnet), ensuring the SQL targets rows with network =
'dash' and sets network = 'mainnet' so records created earlier (e.g., from
zk-fixes) are migrated as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8770ff30-f1b3-4927-b40e-f17ad4c62a52
📒 Files selected for processing (1)
src/database/initialization.rs
…coverage - Add `last_nullifier_sync_timestamp` to CREATE TABLE in `create_shielded_wallet_meta_table` so fresh installs at v33 get the full schema without relying on the migration column-add path - Propagate SQL errors from the column-existence check in `add_nullifier_sync_timestamp_column` instead of silently swallowing them with `unwrap_or(false)` - Add `shielded_notes` and `shielded_wallet_meta` to the tables array in `rename_network_dash_to_mainnet` so the 'dash'→'mainnet' rename covers all network-scoped tables Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add FOREIGN KEY (wallet_seed_hash) REFERENCES wallet(seed_hash) ON DELETE CASCADE to both shielded_notes and shielded_wallet_meta CREATE TABLE statements, matching the pattern used throughout initialization.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
v1.0-devandzk-fixesbranches (both used v28 for different migrations)IF NOT EXISTS/ column-existence checks, safe to re-run on databases that already applied some or all stepsshielded_notes,shielded_wallet_meta) so v1.0-dev databases are ready for the ZK feature branchMigrations included in v33
add_core_wallet_name_column— addscore_wallet_nametowalletsinit_contacts_tables— creates contact request tablescreate_shielded_tables— createsshielded_notestablecreate_shielded_wallet_meta_table— createsshielded_wallet_metatablerename_network_dash_to_mainnet— renames "dash" network to "mainnet"add_wallet_transaction_status_column— addsstatustowallet_transactionsadd_nullifier_sync_timestamp_column— addslast_nullifier_sync_timestamptoshielded_wallet_metaTest plan
create_tables()sets version to 33, all tables existcargo test --all-features --workspacepasses (72 tests verified)🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Chores