refactor: wallet unique constrant#672
Conversation
📝 WalkthroughWalkthroughWallet credential uniqueness is changed from global to tenant-scoped by adding Changes
Sequence DiagramsequenceDiagram
participant Client
participant Router as RPC Router
participant DB as Database
participant Migrator as Migration
Client->>Router: Import wallet (tenantId, credentials)
Router->>Router: Validate request, extract tenantId + credentials
Router->>DB: Query wallets matching (tenantId, xpub, coreDescriptor, mnemonic, nodePubkey)
DB-->>Router: Return matching rows
alt Match found for same tenant
Router-->>Client: Error (InvalidArgument) "same credentials"
else No match for tenant
Router->>DB: Insert new wallet record
DB-->>Router: Insert OK
Router-->>Client: Success (wallet created)
end
Note over Migrator,DB: Migration v18 → v19 recreates wallets table to add tenantId into unique constraint
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
only enforce same wallets within tenants
jackstar12
force-pushed
the
refactor/wallets-unique-constraint
branch
from
496004f to
6c496d7
Compare
| mnemonic VARCHAR, | ||
| subaccount INT, | ||
| salt VARCHAR, | ||
| tenantId INT NOT NULL REFERENCES tenants (id), |
| mnemonic VARCHAR, | ||
| subaccount INT, | ||
| salt VARCHAR, | ||
| tenantId INT REFERENCES tenants (id), |
There was a problem hiding this comment.
In the migration it can be NULL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/database/migration.go`:
- Around line 653-656: The migration must not DROP the live wallets table
because that triggers foreign-key ON DELETE actions and clears swaps.walletId,
reverseSwaps.walletId, and chainSwapsData.walletId; instead, rename the existing
wallets table to a temporary name (e.g., wallets_old), then RENAME new_wallets
TO wallets, and only then DROP TABLE wallets_old — update the SQL block that
currently does "DROP TABLE wallets; ALTER TABLE new_wallets RENAME TO wallets;"
to perform the safe rename sequence so swaps.walletId (and related FKs) continue
pointing to the new wallets table without being nulled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8faab21e-959f-420d-8137-d4bc132ea88c
📒 Files selected for processing (1)
internal/database/migration.go
| UNIQUE (name, tenantId, nodePubkey), | ||
| UNIQUE (tenantId, xpub, coreDescriptor, mnemonic, nodePubkey) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import sqlite3
con = sqlite3.connect(":memory:")
con.executescript("""
CREATE TABLE wallets(
tenantId INT NOT NULL,
xpub TEXT,
coreDescriptor TEXT,
mnemonic TEXT,
nodePubkey TEXT,
UNIQUE (tenantId, xpub, coreDescriptor, mnemonic, nodePubkey)
);
INSERT INTO wallets VALUES (1, 'xpub-1', NULL, NULL, 'node-1');
""")
try:
con.execute("INSERT INTO wallets VALUES (1, 'xpub-1', NULL, NULL, 'node-1')")
print("duplicate insert succeeded")
except Exception as exc:
print("duplicate insert failed:", exc)
PYRepository: BoltzExchange/boltz-client
Length of output: 97
🏁 Script executed:
cat -n internal/database/migration.go | sed -n '640,660p'Repository: BoltzExchange/boltz-client
Length of output: 1014
🏁 Script executed:
cat -n internal/database/database.go | sed -n '200,240p'Repository: BoltzExchange/boltz-client
Length of output: 1316
🏁 Script executed:
rg -i "wallet.*unique\|unique.*wallet" --type-list 2>/dev/null || rg -i "wallet.*unique\|unique.*wallet" -g "*.go" | head -20Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg -A 5 -B 5 "INSERT INTO wallets" --type-list 2>/dev/null || rg -A 5 -B 5 "INSERT INTO wallets" -g "*.go"Repository: BoltzExchange/boltz-client
Length of output: 2418
🏁 Script executed:
rg "coreDescriptor\|mnemonic.*NULL\|xpub.*NULL" -g "*.go" | head -30Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg -i "credential.*type\|wallet.*type" -g "*.go" | grep -i "enum\|const\|type" | head -20Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg "type Wallet struct" -A 20 -g "*.go"Repository: BoltzExchange/boltz-client
Length of output: 8526
🏁 Script executed:
rg "CreateWallet\|NewWallet" -B 2 -A 10 -g "*.go" | head -50Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg "credential\|Credential" -g "*.go" | grep -v "test" | head -20Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg "type WalletCredentials struct" -A 15 -g "*.go"Repository: BoltzExchange/boltz-client
Length of output: 3333
🏁 Script executed:
rg -B 5 -A 15 "struct.*WalletCredentials\|WalletCredentials.*struct" -g "*.go" | head -60Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg -B 5 -A 10 "func.*CreateWallet\|func.*NewWallet" -g "*.go" internal/Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg "xpub.*empty\|mnemonic.*empty\|Xpub\s*=\|Mnemonic\s*=" -g "*.go" | head -30Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg "xpub.*required\|mnemonic.*required" -i -g "*.go"Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg "Mnemonic\|Xpub\|CoreDescriptor" -B 3 -A 3 internal/onchain/wallet.go | head -50Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg -i "credential.*type\|wallet.*type" -B 2 -A 10 internal/ -g "*.go" | grep -A 10 "type"Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg "Validate\|validate" -B 2 -A 5 internal/database/wallet.goRepository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
cat -n internal/database/wallet.go | head -80Repository: BoltzExchange/boltz-client
Length of output: 2712
🏁 Script executed:
rg "CreateWallet" -B 10 -A 15 -g "*.go" | head -100Repository: BoltzExchange/boltz-client
Length of output: 9214
🏁 Script executed:
rg "INSERT INTO wallets" -B 5 -A 15 -g "*.go"Repository: BoltzExchange/boltz-client
Length of output: 3395
🏁 Script executed:
rg "Mnemonic\|Xpub\|CoreDescriptor" -B 2 -A 2 internal/onchain/wallet.go | head -40Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
grep -n "Mnemonic\|Xpub\|CoreDescriptor" internal/onchain/wallet.goRepository: BoltzExchange/boltz-client
Length of output: 1230
🏁 Script executed:
cat internal/onchain/wallet.go | sed -n '1,30p'Repository: BoltzExchange/boltz-client
Length of output: 576
🏁 Script executed:
cat -n internal/onchain/wallet.go | sed -n '160,180p'Repository: BoltzExchange/boltz-client
Length of output: 856
🏁 Script executed:
rg -B 10 -A 10 "credential.*type\|Type.*credential" internal/ -g "*.go" | head -60Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg "xpub\|Xpub" -i -B 2 -A 2 internal/onchain/wallet.goRepository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg "ValidateWalletCredentials" -B 3 -A 3 -g "*.go"Repository: BoltzExchange/boltz-client
Length of output: 2037
🏁 Script executed:
rg "CreateWallet" -B 15 -A 5 -g "*.go" | grep -A 15 "func.*CreateWallet\|CreateWallet.*func" | head -60Repository: BoltzExchange/boltz-client
Length of output: 4213
🏁 Script executed:
find . -name "*.go" -type f -exec grep -l "CreateWallet" {} \; | head -10Repository: BoltzExchange/boltz-client
Length of output: 534
🏁 Script executed:
rg "CreateWallet\|func.*Wallet" -B 5 -A 20 internal/rpcserver/router.go | grep -A 20 "func.*CreateWallet"Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
cat -n internal/rpcserver/router.go | sed -n '1,50p'Repository: BoltzExchange/boltz-client
Length of output: 1914
🏁 Script executed:
rg "NewWallet\|CreateWallet" -B 10 -A 15 internal/rpcserver/router.go | head -100Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg -n "func.*CreateWallet" internal/rpcserver/router.goRepository: BoltzExchange/boltz-client
Length of output: 223
🏁 Script executed:
rg "func.*createWallet\|func.*newWallet" -i internal/rpcserver/router.goRepository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
wc -l internal/rpcserver/router.goRepository: BoltzExchange/boltz-client
Length of output: 104
🏁 Script executed:
cat -n internal/rpcserver/router.go | sed -n '1631,1700p'Repository: BoltzExchange/boltz-client
Length of output: 2900
🏁 Script executed:
rg -n "func.*ImportWallet" internal/rpcserver/router.goRepository: BoltzExchange/boltz-client
Length of output: 209
🏁 Script executed:
cat -n internal/rpcserver/router.go | sed -n '1500,1630p'Repository: BoltzExchange/boltz-client
Length of output: 4999
🏁 Script executed:
cat -n internal/rpcserver/router.go | sed -n '1480,1510p'Repository: BoltzExchange/boltz-client
Length of output: 1351
🏁 Script executed:
rg "GetWallets\|QueryWallets" -B 3 -A 5 internal/database/ -g "*.go" | head -40Repository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
rg "nodePubkey" -B 2 -A 2 internal/onchain/wallet.goRepository: BoltzExchange/boltz-client
Length of output: 52
🏁 Script executed:
cat -n internal/database/database.go | sed -n '210,230p'Repository: BoltzExchange/boltz-client
Length of output: 815
🏁 Script executed:
cat -n internal/database/wallet.go | sed -n '43,65p'Repository: BoltzExchange/boltz-client
Length of output: 814
The UNIQUE constraint has a SQLite NULL-semantics weakness that allows potential duplicate-wallet insertion.
In SQLite, NULL values are treated as distinct in UNIQUE constraints. The constraint UNIQUE (tenantId, xpub, coreDescriptor, mnemonic, nodePubkey) cannot prevent duplicates when nodePubkey or other columns are NULL, even if non-NULL columns match. While ValidateWalletCredentials() ensures CoreDescriptor is populated before database insertion, and application-level duplicate detection exists (lines 1501–1506 in router.go), the database constraint alone provides no guarantee. Partial unique indexes per credential type would be safer and move the enforcement to the schema layer.
| INSERT INTO new_wallets (id, name, currency, nodePubkey, xpub, coreDescriptor, mnemonic, subaccount, salt, tenantId, legacy, lastIndex) | ||
| SELECT id, name, currency, nodePubkey, xpub, coreDescriptor, mnemonic, subaccount, salt, tenantId, legacy, lastIndex FROM wallets; | ||
| DROP TABLE wallets; | ||
| ALTER TABLE new_wallets RENAME TO wallets; |
There was a problem hiding this comment.
Don't drop the live wallets parent table in this migration.
swaps.walletId, reverseSwaps.walletId, and chainSwapsData.walletId all point at wallets(id) with ON DELETE SET NULL (see Line 425-Line 428 and Line 470). In SQLite, DROP TABLE wallets applies foreign-key actions, so this can clear existing walletId links before new_wallets is renamed back. That would silently detach existing swaps from their wallets.
#!/bin/bash
python - <<'PY'
import sqlite3
con = sqlite3.connect(":memory:")
con.execute("PRAGMA foreign_keys = ON")
con.executescript("""
CREATE TABLE wallets(id INTEGER PRIMARY KEY);
CREATE TABLE swaps(
id INTEGER PRIMARY KEY,
walletId INT REFERENCES wallets(id) ON DELETE SET NULL
);
INSERT INTO wallets(id) VALUES (1);
INSERT INTO swaps(id, walletId) VALUES (1, 1);
CREATE TABLE new_wallets(id INTEGER PRIMARY KEY);
INSERT INTO new_wallets(id) VALUES (1);
DROP TABLE wallets;
""")
print(con.execute("SELECT walletId FROM swaps").fetchall())
PYExpected result: [(None,)], which shows the wallet reference is lost before the rename happens.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/database/migration.go` around lines 653 - 656, The migration must
not DROP the live wallets table because that triggers foreign-key ON DELETE
actions and clears swaps.walletId, reverseSwaps.walletId, and
chainSwapsData.walletId; instead, rename the existing wallets table to a
temporary name (e.g., wallets_old), then RENAME new_wallets TO wallets, and only
then DROP TABLE wallets_old — update the SQL block that currently does "DROP
TABLE wallets; ALTER TABLE new_wallets RENAME TO wallets;" to perform the safe
rename sequence so swaps.walletId (and related FKs) continue pointing to the new
wallets table without being nulled.
2 participants
only enforce same wallets within tenants
Summary by CodeRabbit