From c09670530c2ae1a6178ee23ea2b12c4e33fe4ec6 Mon Sep 17 00:00:00 2001
From: Avi <avi@aiolabs.com>
Date: Fri, 11 Sep 2026 15:11:00 -0500
Subject: [PATCH] fix(vault): stop rewriting the vault on every load; clippy
 cleanup

- migrate_vault_signer_modes now reports a change only when the vault
  version actually moves. The unconditional 'changed = true' made
  App::load re-save the vault on every start (harmless, idempotent,
  but wasteful). Per-profile signer_mode normalisation was already a
  no-op: the serde default fills missing fields at parse time and the
  current version serialises it explicitly.
- idempotency test tightened to assert changed == false for a
  current-version vault (previously ducked the question).
- nip46_client.rs: drop clone-on-Copy in get_public_key (clippy).
---
 src/signer/nip46_client.rs |  2 +-
 src/vault.rs               | 54 ++++++++++++++++++--------------------
 2 files changed, 26 insertions(+), 30 deletions(-)

diff --git a/src/signer/nip46_client.rs b/src/signer/nip46_client.rs
index 4221df8..dcab6de 100644
--- a/src/signer/nip46_client.rs
+++ b/src/signer/nip46_client.rs
@@ -1158,7 +1158,7 @@ impl Signer for Nip46ClientSigner {
         // The identity learned from the signer during the handshake — NOT
         // the URI key, which for bunker:// flows is a per-connection comms
         // key. Until the handshake resolves it the session is not usable.
-        inner.identity.clone().ok_or(SigningError::NotConnected)
+        inner.identity.ok_or(SigningError::NotConnected)
     }
 
     async fn sign_event(&self, event: UnsignedEvent) -> Result<Event, SigningError> {
diff --git a/src/vault.rs b/src/vault.rs
index 24d0702..d46ce04 100644
--- a/src/vault.rs
+++ b/src/vault.rs
@@ -356,35 +356,28 @@ pub fn parse_vault(content: &str) -> Result<Vault, AppError> {
 /// left untouched.  Only profiles with the legacy `None` value (or
 /// missing the field entirely) are assigned `Embedded`.
 ///
-/// Returns `true` if any profiles were migrated (i.e. the vault should
-/// be re-saved).
+/// Missing `signer_mode` fields are assigned `Embedded` by the serde
+/// default during deserialization, and every vault at the current
+/// `VAULT_VERSION` serialises `signer_mode` explicitly — so once the
+/// version bump below has been saved, re-saving adds nothing.  A change
+/// is therefore reported only when the version actually moves, instead
+/// of on every load (the old unconditional `changed = true` made
+/// `App::load` rewrite the vault on each start).
+///
+/// Returns `true` if the vault was migrated (i.e. it should be re-saved).
 pub fn migrate_vault_signer_modes(vault: &mut Vault) -> bool {
-    let mut changed = false;
-    for _profile in &mut vault.profiles {
-        // The serde default already handles missing fields during
-        // deserialization, but once loaded, profiles that were stored
-        // before signer_mode was introduced will have the default value.
-        // We write it explicitly so the on-disk format is canonical.
-        //
-        // After the first save, every profile will have an explicit
-        // signer_mode and this becomes a no-op.
-        //
-        // We cannot distinguish "user explicitly set Embedded" from
-        // "serde defaulted to Embedded", so we always write it — this is
-        // safe because Embedded is the correct default and the write is
-        // idempotent.
-        changed = true;
-    }
-    // Also ensure the nip46_connections vector exists (serde default
-    // handles this during deserialization, but we normalise here too).
-    if vault.version < VAULT_VERSION {
-        vault.version = VAULT_VERSION;
-        changed = true;
-    }
+    // Profiles need no per-field work: the serde default already filled
+    // any missing `signer_mode` at parse time and serialization at the
+    // current version writes it explicitly.
+    //
     // Legacy connections without profile_npub (None) are left as-is.
     // Ownership cannot be reliably inferred from active_profile, so these
     // connections remain unusable until the user re-creates them.
-    changed
+    if vault.version < VAULT_VERSION {
+        vault.version = VAULT_VERSION;
+        return true;
+    }
+    false
 }
 
 /// Store (or replace) a NIP-46 connection secret in the vault, keyed by an
@@ -852,11 +845,14 @@ mod tests {
         });
 
         let changed1 = migrate_vault_signer_modes(&mut vault);
-        assert!(changed1, "first migration should report change");
+        // Vault::empty() is already at the current version with an
+        // explicit signer_mode, so migration must report no change —
+        // the old always-true return made App::load rewrite the vault
+        // on every start.
+        assert!(!changed1, "current-version vault should not report change");
 
-        let _changed2 = migrate_vault_signer_modes(&mut vault);
-        // The function always returns true because it normalises the version.
-        // The important thing is that running it twice doesn't corrupt data.
+        let changed2 = migrate_vault_signer_modes(&mut vault);
+        assert!(!changed2, "re-running migration stays a no-op");
         assert_eq!(vault.profiles[0].signer_mode, SignerMode::Embedded);
         assert_eq!(vault.version, VAULT_VERSION);
     }