backend/account: migrate legacy notes upfront

In the past we had different account IDs, and the notes where stored
in different files. To deal with this, we used a list of notes objects
per account to be able to fetch notes from all files.

This commit changes this to migrate/merge legacy note files so we only
have one notes file per account. This is conceptually easier and
simplifies the upcoming notes export feature.

This also fixes a bug where the rates updater event is not propagated
for accounts with account number > 0 - Initialize() returned early
with `return nil` in that case.

NotesData is renamed to Data as the linter thougth it was a bad name:

> backend/accounts/notes/notes.go:30:6: exported: type name will be used as notes.NotesData by other packages, and that stutters; consider calling this Data (revive)

Author: benma

backend/accounts/baseaccount.go

@@ -71,12 +71,7 @@ type BaseAccount struct {
 	offline error
 
 	// notes handles transaction notes.
-	//
-	// It is a slice for migration purposes: from v4.27.0 to v4.28.0, the account identifiers
-	// changed. The slice contains all possible instances of where notes are stored. The first
-	// element is the newest, and other elements are notes stored under legacy names. After
-	// `Initialize()`, this will always have at least one element.
-	notes []*notes.Notes
+	notes *notes.Notes
 
 	proposedTxNote   string
 	proposedTxNoteMu sync.Mutex
@@ -151,10 +146,28 @@ func (account *BaseAccount) Initialize(accountIdentifier string) error {
 	if err != nil {
 		return err
 	}
-	account.notes = []*notes.Notes{txNotes}
+	account.notes = txNotes
 
-	// Append legacy notes (notes stored in files based on obsolete account identifiers). Account
-	// identifiers changed from v4.27.0 to v4.28.0.
+	if err := account.migrateLegacyNotes(); err != nil {
+		return err
+	}
+
+	// An account syncdone event is generated when new rates are available. This allows the frontend
+	// to reload the relevant data.
+	if account.config.RateUpdater != nil {
+		account.config.RateUpdater.Observe(func(e observable.Event) {
+			if e.Subject == rates.RatesEventSubject {
+				account.config.OnEvent(types.EventSyncDone)
+			}
+		})
+	}
+
+	return nil
+}
+
+// Migrate legacy notes (notes stored in files based on obsolete account identifiers). Account
+// identifiers changed from v4.27.0 to v4.28.0.
+func (account *BaseAccount) migrateLegacyNotes() error {
 	if len(account.Config().Config.SigningConfigurations) == 0 {
 		return nil
 	}
@@ -199,18 +212,11 @@ func (account *BaseAccount) Initialize(accountIdentifier string) error {
 		if err != nil {
 			return err
 		}
-		account.notes = append(account.notes, legacyNotes)
-	}
-
-	// An account syncdone event is generated when new rates are available. This allows the frontend to reload the relevant data.
-	if account.config.RateUpdater != nil {
-		account.config.RateUpdater.Observe(func(e observable.Event) {
-			if e.Subject == rates.RatesEventSubject {
-				account.config.OnEvent(types.EventSyncDone)
-			}
-		})
+		if err := account.notes.MergeLegacy(legacyNotes); err != nil {
+			return err
+		}
+		_ = os.Remove(notesFile)
 	}
-
 	return nil
 }
 
@@ -235,31 +241,17 @@ func (account *BaseAccount) GetAndClearProposedTxNote() string {
 
 // SetTxNote implements accounts.Account.
 func (account *BaseAccount) SetTxNote(txID string, note string) error {
-	// The notes slice is guaranteed to have at least one element by BaseAccount.Initialize.
-	if err := account.notes[0].SetTxNote(txID, note); err != nil {
+	if err := account.notes.SetTxNote(txID, note); err != nil {
 		return err
 	}
-	// Delete the notes in legacy files. Don't really care if it fails.
-	for i, notes := range account.notes[1:] {
-		if err := notes.SetTxNote(txID, ""); err != nil {
-			account.log.WithError(err).Errorf("Can't delete a note from a legacy file idx=%d", i)
-		}
-	}
 	// Prompt refresh.
 	account.config.OnEvent(types.EventStatusChanged)
 	return nil
 }
 
 // TxNote fetches a note for a transcation. Returns the empty string if no note was found.
 func (account *BaseAccount) TxNote(txID string) string {
-	// Take the first note we can find. The first slice element is the regular location of notes,
-	// the other elements lookup notes in legacy locations, so they are not lost when upgrading.
-	for _, notes := range account.notes {
-		if note := notes.TxNote(txID); note != "" {
-			return note
-		}
-	}
-	return ""
+	return account.notes.TxNote(txID)
 }
 
 // ExportCSV implements accounts.Account.

backend/accounts/baseaccount_test.go

@@ -74,28 +74,40 @@ func TestBaseAccount(t *testing.T) {
 	// This tests notes migration from v4.27.0 to v4.28.0.
 	require.NoError(t,
 		os.WriteFile(
-			path.Join(cfg.NotesFolder, "account-54b4597c3a5c48177ef2b12c97e0cb30b6fef0b431e7821675b17704c330ce5d-tbtc.json"),
-			[]byte(`{"transactions": { "legacy-1": "legacy note in unified account" }}`),
+			path.Join(cfg.NotesFolder, "test-account-identifier.json"),
+			[]byte(`{"transactions": { "conflict": "conflict note new" }}`),
+			0666,
+		),
+	)
+
+	legacyNotesFilename1 := path.Join(cfg.NotesFolder, "account-54b4597c3a5c48177ef2b12c97e0cb30b6fef0b431e7821675b17704c330ce5d-tbtc.json")
+	legacyNotesFilename2 := path.Join(cfg.NotesFolder, "account-989b84ec36f0b84e7926f9f5715e4b59a0592993b1fa4b70836addbcb0cb6e09-tbtc-p2pkh.json")
+	legacyNotesFilename3 := path.Join(cfg.NotesFolder, "account-e60b99507ba983d522f15932dbe1214e99de13c56e2bea75ed9c285f7c013117-tbtc-p2wpkh.json")
+	legacyNotesFilename4 := path.Join(cfg.NotesFolder, "account-9e779e0d49e77236f0769e0bab2fd656958d3fd023dce2388525ee66fead88bb-tbtc-p2wpkh-p2sh.json")
+	require.NoError(t,
+		os.WriteFile(
+			legacyNotesFilename1,
+			[]byte(`{"transactions": { "legacy-1": "legacy note in unified account", "conflict": "conflict note old" }}`),
 			0666,
 		),
 	)
 	require.NoError(t,
 		os.WriteFile(
-			path.Join(cfg.NotesFolder, "account-989b84ec36f0b84e7926f9f5715e4b59a0592993b1fa4b70836addbcb0cb6e09-tbtc-p2pkh.json"),
+			legacyNotesFilename2,
 			[]byte(`{"transactions": { "legacy-2": "legacy note in split account, p2pkh" }}`),
 			0666,
 		),
 	)
 	require.NoError(t,
 		os.WriteFile(
-			path.Join(cfg.NotesFolder, "account-e60b99507ba983d522f15932dbe1214e99de13c56e2bea75ed9c285f7c013117-tbtc-p2wpkh.json"),
+			legacyNotesFilename3,
 			[]byte(`{"transactions": { "legacy-3": "legacy note in split account, p2wpkh" }}`),
 			0666,
 		),
 	)
 	require.NoError(t,
 		os.WriteFile(
-			path.Join(cfg.NotesFolder, "account-9e779e0d49e77236f0769e0bab2fd656958d3fd023dce2388525ee66fead88bb-tbtc-p2wpkh-p2sh.json"),
+			legacyNotesFilename4,
 			[]byte(`{"transactions": { "legacy-4": "legacy note in split account, p2wpkh-p2sh" }}`),
 			0666,
 		),
@@ -150,6 +162,17 @@ func TestBaseAccount(t *testing.T) {
 	})
 
 	t.Run("notes", func(t *testing.T) {
+		// Legacy note files have been deleted after migration.
+		for _, filename := range []string{
+			legacyNotesFilename1,
+			legacyNotesFilename2,
+			legacyNotesFilename3,
+			legacyNotesFilename4,
+		} {
+			_, err := os.Stat(filename)
+			require.True(t, os.IsNotExist(err))
+		}
+
 		require.Equal(t, "", account.GetAndClearProposedTxNote())
 		account.ProposeTxNote("test note")
 		require.Equal(t, "test note", account.GetAndClearProposedTxNote())
@@ -161,21 +184,14 @@ func TestBaseAccount(t *testing.T) {
 		require.Equal(t, "another test note", account.TxNote("test-tx-id"))
 
 		// Test notes migration from v4.27.0 to v4.28.0
+		require.Equal(t, "conflict note new", account.TxNote("conflict"))
 		require.Equal(t, "legacy note in unified account", account.TxNote("legacy-1"))
 		require.Equal(t, "legacy note in split account, p2pkh", account.TxNote("legacy-2"))
 		require.Equal(t, "legacy note in split account, p2wpkh", account.TxNote("legacy-3"))
 		require.Equal(t, "legacy note in split account, p2wpkh-p2sh", account.TxNote("legacy-4"))
 		// Setting a note sets it in the main notes file, and wipes it out in legacy note files.
 		require.NoError(t, account.SetTxNote("legacy-1", "updated legacy note"))
 		require.Equal(t, "updated legacy note", account.TxNote("legacy-1"))
-		contents, err := os.ReadFile(path.Join(cfg.NotesFolder, "account-54b4597c3a5c48177ef2b12c97e0cb30b6fef0b431e7821675b17704c330ce5d-tbtc.json"))
-		require.NoError(t, err)
-		require.JSONEq(t, `{"transactions":{}}`, string(contents))
-
-		// Test that the notes were persisted under the right file name with the right contents.
-		contents, err = os.ReadFile(path.Join(cfg.NotesFolder, "test-account-identifier.json"))
-		require.NoError(t, err)
-		require.JSONEq(t, `{"transactions":{"legacy-1": "updated legacy note", "test-tx-id": "another test note"}}`, string(contents))
 	})
 
 	t.Run("exportCSV", func(t *testing.T) {

backend/accounts/notes/notes.go

@@ -26,8 +26,8 @@ import (
 // maxNoteLen is the maximum length per note.
 const maxNoteLen = 1024
 
-// NotesData is the notes JSON data serialized to disk.
-type notesData struct {
+// Data is the notes JSON data serialized to disk.
+type Data struct {
 	// More fields to be added when we can label more stuff, e.g. receive addresses, utxos, etc.
 
 	// a map of transaction ID to transaction note.
@@ -36,23 +36,23 @@ type notesData struct {
 
 // read deserializes the json files into notes. If the file does not exist yet, no error is
 // returned, and the struct is retruned with default values.
-func read(filename string) (*notesData, error) {
+func read(filename string) (*Data, error) {
 	file, err := os.Open(filename)
 	if err != nil {
 		if os.IsNotExist(err) {
-			return &notesData{}, nil
+			return &Data{}, nil
 		}
 		return nil, errp.WithStack(err)
 	}
 	defer file.Close() //nolint:errcheck
-	var notes notesData
+	var notes Data
 	if err := json.NewDecoder(file).Decode(&notes); err != nil {
 		return nil, errp.WithStack(err)
 	}
 	return &notes, nil
 }
 
-func write(data *notesData, filename string) error {
+func write(data *Data, filename string) error {
 	file, err := os.OpenFile(filename, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600)
 	if err != nil {
 		return err
@@ -69,7 +69,7 @@ func write(data *notesData, filename string) error {
 // Notes is a high level helper for notes, allowing you to read and set notes for transactions.
 type Notes struct {
 	filename string
-	data     *notesData
+	data     *Data
 	dataMu   sync.RWMutex
 }
 
@@ -117,3 +117,29 @@ func (notes *Notes) TxNote(txID string) string {
 
 	return notes.data.TransactionNotes[txID]
 }
+
+// Data retrieves all stored notes. You must not modify the returned object.
+func (notes *Notes) Data() *Data {
+	notes.dataMu.RLock()
+	defer notes.dataMu.RUnlock()
+	return notes.data
+}
+
+// MergeLegacy merges the notes from an older/legacy notes file. Current/new notes take priority in
+// case of conflict.
+func (notes *Notes) MergeLegacy(legacy *Notes) error {
+	notes.dataMu.Lock()
+	defer notes.dataMu.Unlock()
+
+	if notes.data.TransactionNotes == nil {
+		notes.data.TransactionNotes = map[string]string{}
+	}
+
+	legacyData := legacy.Data()
+	for txID, note := range legacyData.TransactionNotes {
+		if _, ok := notes.data.TransactionNotes[txID]; !ok {
+			notes.data.TransactionNotes[txID] = note
+		}
+	}
+	return write(notes.data, notes.filename)
+}

backend/accounts/notes/notes_test.go

@@ -39,6 +39,15 @@ func TestNotes(t *testing.T) {
 	require.NoError(t, notes.SetTxNote("tx-id-2", "note for tx-id-2"))
 	require.Equal(t, "note for tx-id-1", notes.TxNote("tx-id-1"))
 	require.Equal(t, "note for tx-id-2", notes.TxNote("tx-id-2"))
+
+	require.Equal(t,
+		&Data{
+			TransactionNotes: map[string]string{
+				"tx-id-1": "note for tx-id-1",
+				"tx-id-2": "note for tx-id-2",
+			},
+		},
+		notes.Data())
 }
 
 // TestNotesPersisted checks that notes are persisted.
@@ -68,3 +77,40 @@ func TestMaxLen(t *testing.T) {
 	require.NoError(t, notes.SetTxNote("tx-id", strings.Repeat("x", 1024)))
 	require.Error(t, notes.SetTxNote("tx-id", strings.Repeat("x", 1025)))
 }
+
+func TestMergeLegacy(t *testing.T) {
+	filename := test.TstTempFile("account-notes")
+	notes, err := LoadNotes(filename)
+	require.NoError(t, err)
+	require.NoError(t, notes.SetTxNote("tx-id-1", "note for tx-id-1"))
+	require.NoError(t, notes.SetTxNote("tx-id-2", "note for tx-id-2"))
+
+	legacyNotes, err := LoadNotes(test.TstTempFile("legacy-notes"))
+	require.NoError(t, err)
+	require.NoError(t, legacyNotes.SetTxNote("tx-id-1", "legacy note for tx-id-1"))
+	require.NoError(t, legacyNotes.SetTxNote("tx-id-3", "legacy note for tx-id-3"))
+
+	require.NoError(t, notes.MergeLegacy(legacyNotes))
+	require.Equal(t,
+		&Data{
+			TransactionNotes: map[string]string{
+				"tx-id-1": "note for tx-id-1",
+				"tx-id-2": "note for tx-id-2",
+				"tx-id-3": "legacy note for tx-id-3",
+			},
+		},
+		notes.Data())
+
+	// Check that the merged notes were persisted.
+	notes, err = LoadNotes(filename)
+	require.NoError(t, err)
+	require.Equal(t,
+		&Data{
+			TransactionNotes: map[string]string{
+				"tx-id-1": "note for tx-id-1",
+				"tx-id-2": "note for tx-id-2",
+				"tx-id-3": "legacy note for tx-id-3",
+			},
+		},
+		notes.Data())
+}