Migrate vault entries to new schema #2092

ekzyis · 2025-04-11T02:57:44Z

Description

This PR replaces the existing VaultEntry table which links to a row in the generic Wallet table (and to a row in the users table) with a new table Vault where columns in the specific wallet tables like WalletNWC link to it.

TODO:

Checklist

Are your changes backwards compatible? Please answer below:

Yes. All rows from VaultEntry are migrated to Vault with a link to them from their respective wallet tables and all LNC and WebLN rows from Wallet are now also inserted into their new wallet tables.

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:

8 (7a4832a). Tested saving recv-only/send-only/recv+send of following wallets with/without device sync:

CLN
NWC
LNbits
LND
lightning address
LNC
WebLN
phoenixd

(= every wallet except Blink)
since my internet is kind of bad here (attaching NWC times out Phoenixd
Tried to test migration with production data but metabase CSV export is broken. It returns NULL for empty strings so it broke on users."vaultKeyHash" being NULL.¹ Didn't bother further if the import already doesn't work because of something like this.

Okay, I haven't tested 727d931 as much as I did 7a4832a but it should still be a solid 7 at least!

I tested WebLN and various combinations of LNbits with device sync and not etc.

For frontend changes: Tested on mobile, light and dark mode? Please answer below:

n/a

Did you introduce any new environment variables? If so call them out explicitly here:

no

not sure why this isn't a nullable column but uses an empty string as the default but I'll look into this in a future PR ... ↩

lib/object.js

prisma/migrations/20250411012334_vault_refactor/migration.sql

prisma/schema.prisma

wallets/blink/index.js

wallets/vault.js

ekzyis · 2025-04-21T00:40:32Z

api/resolvers/vault.js

-          where: { userId_key: { userId: me.id, key: entry.key } },
-          data: { value: entry.value, iv: entry.iv }
-        }))
+      const wallets = await models.wallet.findMany({ where: { userId: me.id } })


I think fetching the wallets outside of the transaction is fine since it's a read-only operation.

Else this needs to become an interactive transaction 👀

I tried to do this with a bulk update query instead of individual updates per wallet but Prisma doesn't seem to support that and I don't know how to update across multiple subtables via raw SQL depending on the wallet type.

Regardless of whether this is interactive, this code is/was vulnerable to race conditions if a wallet is upserted while the vault key is updated.

Okay, I assume this is blocking this PR

Fixed in bc7c722

This wasn't blocking because even after your changes the code is vulnerable to a race "if a wallet is upserted while the vault key is updated." Your change only protects against hash change races on the user row.

e.g.

tx1: read wallets (wallets with ids [1,2])
tx2: insert wallet (wallet [3])
tx2: commit
tx1: update hash
tx1: commit

Wallet [3] is broken because it's encrypted with a different key.

The above race is very unlikely to occur, but that's why it wasn't blocking. The real fix is nontrivial too: the only way to afaict is to give each vault entry a foreign key constraint on the correct key hash. That way, if the hash is updated any concurrent inserts will fail.

Both transactions read the old vault key hash first, so one of them should fail when they try to update it at the end because they won't find the row anymore, no?

edit: Oh, I think I see what you mean. We can insert wallets without ever checking the current vault key hash.

ekzyis · 2025-04-24T18:59:59Z

Oh, forgot to put this out of draft yesterday

huumn · 2025-04-28T21:30:44Z

wallets/vault.js

+export function hasVault (wallet) {
+  const def = getWalletByType(wallet.type)
+  const vaultNames = vaultFieldNames(def)
+  return vaultNames.some(name => get(wallet, `wallet.${name}Id`))


Couldn't this just be written as

wallet.wallet[`${name}Id`]

The only other usage is on 93 which can be written as:

wallet[def.walletField][name]

Where else do you imagine using this? And why?

Mhh, I don't really remember.

Might had something to do with me wanting to have a move function in vaultNewSchematoTypedef since it essentially requires "moving" object values around but I didn't end up using the move I wrote after I wrote some primitives (get, set, remove) for such a object helper function.

I'll remove it.

huumn

My main gripe is the JSON trigger stuff. We'll need to figure out what to do there. Regardless of that, I'll need to test the migration before merging this.

You've described this PR as an intermediate step to something else so I understand this isn't "the end." I think it might be helpful for me (or any other reviewer) for you to describe "the end" that you're after so you can get feedback on it and the steps (like this) that you'll be taking to get there. Like, what's next after this PR? What will this PR allow you to objectively do that you weren't able to before? And, what will that allow you to do? And so on.

huumn · 2025-04-28T22:33:26Z

wallets/vault.js

+function includeFragment (wallet) {
+  const include = walletDefs.reduce((acc, def) => {
+    const names = vaultFieldNames(def)
+    if (names.length === 0) return acc
+
+    return {
+      ...acc,
+      [def.walletField]: {
+        include: names.reduce((acc2, name) => ({
+          ...acc2,
+          [name]: true
+        }), {})
+      }
+    }
+  }, {})
+
+  if (wallet) {
+    const def = getWalletByType(wallet.type)
+    const names = vaultFieldNames(def)
+    if (names.length === 0) return {}
+    return { [def.walletField]: include[def.walletField] }
+  }
+
+  return include
+}


The reason we do the JSON trigger is to avoid doing a join with every wallet table like this.

If we're not going to use the JSON trigger, we should remove it. Alternatively, we should remove this N-table join and refactor the JSON triggers.

The con of the N-table join is that we pay the variable (and expected to grow) cost of reading all the wallets very frequently (even your update code does the joins). The pro is that we don't have to maintain triggers.

The con of the trigger is that maintaining the triggers can't be done in javascript and will require DB introspection to work with the changes you are trying to make in this PR (i.e. how do we know the wallet column references a vault entry? Probably by checking for a foreign key relationship.). The pro is that wallets are trivial to read from the DB both in terms of performance and the DB query to retrieve them.

tldr your changes can probably be made to make only reads expensive and writes efficient - relatively. The trigger can be made to make reads efficient and writes expensive - relatively.

ekzyis added the wallets label Apr 11, 2025

ekzyis marked this pull request as draft April 11, 2025 02:57

ekzyis force-pushed the wallet-refactor-migrate-vault branch 5 times, most recently from 5729ce6 to b35f1f7 Compare April 15, 2025 17:13

ekzyis mentioned this pull request Apr 15, 2025

Remove unused wallet resolvers #2104

Merged

ekzyis force-pushed the wallet-refactor-migrate-vault branch 8 times, most recently from 1423528 to 5f09dea Compare April 16, 2025 05:31

ekzyis commented Apr 16, 2025

View reviewed changes

lib/object.js Outdated Show resolved Hide resolved

prisma/migrations/20250411012334_vault_refactor/migration.sql Outdated Show resolved Hide resolved

prisma/schema.prisma Outdated Show resolved Hide resolved

wallets/blink/index.js Outdated Show resolved Hide resolved

ekzyis mentioned this pull request Apr 16, 2025

Remove unused vault resolvers #2110

Merged

ekzyis force-pushed the wallet-refactor-migrate-vault branch 9 times, most recently from c56113b to 7a4832a Compare April 19, 2025 05:46

ekzyis commented Apr 19, 2025

View reviewed changes

wallets/vault.js Outdated Show resolved Hide resolved

ekzyis marked this pull request as ready for review April 21, 2025 00:30

ekzyis commented Apr 21, 2025

View reviewed changes

ekzyis requested a review from huumn April 21, 2025 00:45

ekzyis marked this pull request as draft April 22, 2025 17:36

ekzyis mentioned this pull request Apr 23, 2025

Give people zapping sats more rewards than those that zap CCs #2111

Open

ekzyis force-pushed the wallet-refactor-migrate-vault branch 2 times, most recently from 5fa5ab2 to bc7c722 Compare April 24, 2025 03:08

ekzyis marked this pull request as ready for review April 24, 2025 19:00

ekzyis added 7 commits April 26, 2025 16:26

Migrate existing vault entries to new schema

6d21b6e

Read+write new vault schema

52f19ef

Drop VaultEntry table

cdae066

Refactor vaultPrismaFragments

7450d69

Remove wrong comment

fe7105a

Remove TODO

a24f8cb

Fix possible race condition on update of vault key

efe7c1d

ekzyis force-pushed the wallet-refactor-migrate-vault branch from bc7c722 to efe7c1d Compare April 26, 2025 21:26

Merge branch 'master' into wallet-refactor-migrate-vault

c00d39e

huumn reviewed Apr 28, 2025

View reviewed changes

huumn requested changes Apr 28, 2025

View reviewed changes

Add wallets/SPEC.md

1b85cde

ekzyis force-pushed the wallet-refactor-migrate-vault branch from f254015 to 1b85cde Compare April 29, 2025 05:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate vault entries to new schema #2092

Migrate vault entries to new schema #2092

ekzyis commented Apr 11, 2025 •

edited

Loading

ekzyis Apr 21, 2025 •

edited

Loading

huumn Apr 21, 2025

ekzyis Apr 22, 2025

ekzyis Apr 23, 2025 •

edited

Loading

huumn Apr 28, 2025 •

edited

Loading

huumn Apr 28, 2025 •

edited

Loading

ekzyis Apr 28, 2025 •

edited

Loading

ekzyis commented Apr 24, 2025

huumn Apr 28, 2025

ekzyis Apr 28, 2025

huumn left a comment

huumn Apr 28, 2025

Migrate vault entries to new schema #2092

Are you sure you want to change the base?

Migrate vault entries to new schema #2092

Conversation

ekzyis commented Apr 11, 2025 • edited Loading

Description

Checklist

Footnotes

ekzyis Apr 21, 2025 • edited Loading

Choose a reason for hiding this comment

huumn Apr 21, 2025

Choose a reason for hiding this comment

ekzyis Apr 22, 2025

Choose a reason for hiding this comment

ekzyis Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

huumn Apr 28, 2025 • edited Loading

Choose a reason for hiding this comment

huumn Apr 28, 2025 • edited Loading

Choose a reason for hiding this comment

ekzyis Apr 28, 2025 • edited Loading

Choose a reason for hiding this comment

ekzyis commented Apr 24, 2025

huumn Apr 28, 2025

Choose a reason for hiding this comment

ekzyis Apr 28, 2025

Choose a reason for hiding this comment

huumn left a comment

Choose a reason for hiding this comment

huumn Apr 28, 2025

Choose a reason for hiding this comment

ekzyis commented Apr 11, 2025 •

edited

Loading

ekzyis Apr 21, 2025 •

edited

Loading

ekzyis Apr 23, 2025 •

edited

Loading

huumn Apr 28, 2025 •

edited

Loading

huumn Apr 28, 2025 •

edited

Loading

ekzyis Apr 28, 2025 •

edited

Loading