Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix send-only wallet save after device sync enabled #1732

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Dec 17, 2024

Description

Using NWC as the example wallet here.

If you have a NWC send-only config for a wallet and then enable device sync, the mutation in syncLocalWallets only creates a row in Wallet and VaultEntry but not in WalletNWC.

If you then try to save again, the backend will see that there is a row for the wallet in Wallet and will try to update WalletNWC. This fails because there was no such row created during sync to server.

This PR fixes this by always creating a row in WalletNWC if Wallet was created.


update: Okay, this breaks WebLN though as the comment // client only wallets has no walletData indicated why there was this check for existing wallet data. Added as TODO.

Maybe the fix is to also pass an empty receive config during local sync since that's what we also do on regular saves.


update: decided to use a nested upsert during wallet.update

Video of bug

2024-12-17.19-24-13.mp4

Additional Context

  • There is still a bug where if you now disconnect from the user vault, the row in VaultEntry is deleted but the row in Wallet isn't so you now have an enabled wallet with an empty config. Added as TODO.

TODO

Checklist

Are your changes backwards compatible? Please answer below:

yes

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:

5. I am not entirely sure if I tested every combination of send-only, send+recv, recv-only and device sync enabled/disabled with NWC but I tried and I saw no issues. That these changes fix the issue without breaking anything else also make sense to me.

Also tested enabling/disabling WebLN with and without device sync since a previous attempt broke it.

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

@ekzyis ekzyis marked this pull request as draft December 17, 2024 19:46
@ekzyis ekzyis force-pushed the fix-send-only-wallet-after-device-sync-enabled branch from 44524cc to 5dc58ca Compare December 19, 2024 14:18
@ekzyis ekzyis marked this pull request as ready for review December 19, 2024 15:24
@ekzyis ekzyis force-pushed the fix-send-only-wallet-after-device-sync-enabled branch from 5dc58ca to 8d591a3 Compare December 19, 2024 15:24
@huumn
Copy link
Member

huumn commented Dec 19, 2024

I tried to figure out where this could go wrong, but I think it's right.

I've been wondering if there's a way we can make this less crazy. I think part of the problem is that the relationship between vault entries and wallets isn't strong enough. Maybe we need to have a column in the wallets for each vault entry, even if it's just a foreign key.

model WalletNWC {
  id         Int      @id @default(autoincrement())
  walletId   Int      @unique
  wallet     Wallet   @relation(fields: [walletId], references: [id], onDelete: Cascade)
  createdAt  DateTime @default(now()) @map("created_at")
  updatedAt  DateTime @default(now()) @updatedAt @map("updated_at")
  nwcUrlRecv String?
  nwcUrlSendVaultEntryId Int? 
  nwcUrlSendVaultEntry VaultEntry @relation(fields: [nwcUrlSendVaultEntryId], references: [id], onDelete: SetNull)
}

@huumn huumn merged commit fdbe14d into master Dec 19, 2024
6 checks passed
@huumn huumn deleted the fix-send-only-wallet-after-device-sync-enabled branch December 19, 2024 17:31
@ekzyis
Copy link
Member Author

ekzyis commented Dec 19, 2024

I've been wondering if there's a way we can make this less crazy.

I think first we need to fix the frontend because of #1743 👀

It seems like too many things depend on each other so disabling device sync is brittle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants