From c26ef89a9321961789cc400f115dc3963b8557b2 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Sun, 21 Jun 2026 20:52:20 -0400 Subject: [PATCH] fix(sync): empty wallet save must not wipe the synced copy on relays MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Data-loss bug: on a fresh device the user opened the Wallet pane (field empty, wallet not yet restored), clicked Save, and the "removed" branch called walletSync.disable() — publishing a {cleared:true} note that destroyed the good encrypted copy another device relied on. (Confirmed in logs: a kind-30078 "bitvid:nwc" {cleared:true} event signed right after login.) An empty save happens routinely (saving before hydration/restore), so treating it as a deliberate disconnect-that-wipes-the-remote is wrong. Removed the auto-clear from the empty-save branch: the remote synced note is now changed ONLY via the explicit sync toggle (enable/disable) or a successful save (push). An empty save clears just the LOCAL wallet. tests/wallet-sync-no-clobber.test.mjs: empty save does not call disable/push; a real save still pushes. Mutation-verified (re-adding the disable fails the test). Co-Authored-By: Claude Opus 4.8 --- js/ui/profileModal/ProfileWalletController.js | 19 ++-- tests/wallet-sync-no-clobber.test.mjs | 86 +++++++++++++++++++ 2 files changed, 93 insertions(+), 12 deletions(-) create mode 100644 tests/wallet-sync-no-clobber.test.mjs diff --git a/js/ui/profileModal/ProfileWalletController.js b/js/ui/profileModal/ProfileWalletController.js index 428d8075..018f109e 100644 --- a/js/ui/profileModal/ProfileWalletController.js +++ b/js/ui/profileModal/ProfileWalletController.js @@ -631,18 +631,13 @@ export class ProfileWalletController { finalVariant = "info"; this.mainController.showStatus("Wallet connection removed."); context.reason = "cleared"; - // Don't leave a now-removed spending URI lingering (encrypted) on relays. - const sync = this.getWalletSyncService(); - if (sync.isEnabled(normalizedActive)) { - try { - await sync.disable(normalizedActive); - } catch (syncError) { - devLogger.warn( - "[ProfileModal] Wallet sync clear after disconnect failed:", - syncError - ); - } - } + // NOTE: deliberately do NOT clear the remote synced note here. An empty + // save happens routinely (e.g. saving before the wallet has hydrated / + // been restored on a fresh device), and wiping the encrypted note on + // relays would destroy a good copy that another device relies on. The + // remote note is only ever changed via the explicit sync toggle + // (enable/disable) or a successful save (push). Toggling sync off is the + // single, deliberate way to clear the remote. } context.success = true; } catch (error) { diff --git a/tests/wallet-sync-no-clobber.test.mjs b/tests/wallet-sync-no-clobber.test.mjs new file mode 100644 index 00000000..a9908bb5 --- /dev/null +++ b/tests/wallet-sync-no-clobber.test.mjs @@ -0,0 +1,86 @@ +// Regression (data-loss): saving the Wallet pane with an EMPTY URI must NOT wipe +// the encrypted synced copy on relays. This happened on a fresh device — the user +// opened the pane (field empty, wallet not yet restored), clicked Save, and the +// "removed" branch called walletSync.disable() which published a {cleared:true} +// note, destroying the good remote copy another device relied on. +// +// Correct behavior: an empty save only clears the LOCAL wallet; the remote synced +// note is changed solely via the explicit sync toggle (enable/disable) or a +// successful save (push). + +import "./test-helpers/setup-localstorage.mjs"; +import assert from "node:assert/strict"; +import test from "node:test"; +import { ProfileWalletController } from "../js/ui/profileModal/ProfileWalletController.js"; + +const PUBKEY = "a".repeat(64); + +function makeController() { + const calls = { disable: 0, push: 0 }; + const mainController = { + normalizeHexPubkey: (v) => (typeof v === "string" ? v : ""), + getActivePubkey: () => PUBKEY, + showSuccess: () => {}, + showStatus: () => {}, + showError: () => {}, + callbacks: { onWalletSave: () => {} }, + services: { nwcSettings: {} }, + }; + const controller = new ProfileWalletController(mainController); + + // Stub the instance methods handleWalletSave depends on (no real DOM/services). + controller.isWalletBusy = () => false; + controller.getWalletFormValues = () => ({ uri: "", defaultZap: null, error: null }); + controller.validateWalletUri = () => ({ valid: true, sanitized: "", message: "" }); + controller.persistWalletSettings = async () => ({}); + controller.setWalletPaneBusy = () => {}; + controller.updateWalletStatus = () => {}; + controller.refreshWalletPaneState = () => {}; + + // Sync is opted-in on this device (the dangerous precondition), with spies. + controller.getWalletSyncService = () => ({ + isEnabled: () => true, + disable: async () => { + calls.disable += 1; + return { ok: true }; + }, + push: async () => { + calls.push += 1; + return { ok: true }; + }, + }); + + return { controller, calls }; +} + +test("empty save does NOT clear the remote synced wallet note", async () => { + const { controller, calls } = makeController(); + const result = await controller.handleWalletSave(); + + assert.equal(result.reason, "cleared", "empty save takes the removal branch"); + assert.equal( + calls.disable, + 0, + "an empty save must NOT call walletSync.disable() (no remote {cleared} publish)", + ); + assert.equal(calls.push, 0, "an empty save must not push either"); +}); + +test("a real save with a URI DOES push the synced copy (sanity)", async () => { + const { controller, calls } = makeController(); + controller.getWalletFormValues = () => ({ + uri: "nostr+walletconnect://abc", + defaultZap: null, + error: null, + }); + controller.validateWalletUri = () => ({ + valid: true, + sanitized: "nostr+walletconnect://abc", + message: "", + }); + + const result = await controller.handleWalletSave(); + assert.equal(result.reason, "saved"); + assert.equal(calls.push, 1, "a real save re-pushes when sync is enabled"); + assert.equal(calls.disable, 0, "a real save never clears the remote"); +});