backend/account/notes: restore transaction notes lost in migration

Transaction notes are stored in json files named using the account
identifier. In v4.28.0, account identifiers changed as part of the
multi-accounts feature. It used to contain the hash of the signing
configurations. As a result, transaction notes set before were not
shown anymore, as the wrong file was loaded.

This patch restores the legacy transaction notes by loading the legacy
transaction note files.

In v4.27.0, you could have a unified account and split them into
individual accounts (one per signing configuration). Transaction notes
were not shared between these accounts even them. In v4.28.0, we
removed the split-account setting, so we consider notes from split
accounts as well - otherwise they would be lost for good.

Author: benma

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Changelog
 
 ## [Unreleased]
+- Restore lost transaction notes when ugprading to v4.28.0.
 - Improve error message when EtherScan responds with a rate limit error.
 
 ## 4.28.0 [released 2021-05-27]

backend/accounts/account.go

@@ -20,7 +20,6 @@ import (
 
 	"github.com/btcsuite/btcd/wire"
 	"github.com/btcsuite/btcutil"
-	"github.com/digitalbitbox/bitbox-wallet-app/backend/accounts/notes"
 	"github.com/digitalbitbox/bitbox-wallet-app/backend/coins/coin"
 	"github.com/digitalbitbox/bitbox-wallet-app/backend/signing"
 	"github.com/digitalbitbox/bitbox-wallet-app/util/observable"
@@ -84,7 +83,7 @@ type Interface interface {
 	CanVerifyAddresses() (bool, bool, error)
 	VerifyAddress(addressID string) (bool, error)
 
-	Notes() *notes.Notes
+	TxNote(txID string) string
 	// ProposeTxnote stores a note. The note is is persisted in the notes database upon calling
 	// SendTx(). This function must be called before `SendTx()`.
 	ProposeTxNote(string)

backend/accounts/baseaccount.go

@@ -18,6 +18,7 @@ import (
 	"encoding/csv"
 	"fmt"
 	"io"
+	"os"
 	"path"
 	"sync"
 	"time"
@@ -65,17 +66,26 @@ type BaseAccount struct {
 	synced  bool
 	offline error
 
-	notes *notes.Notes
+	// 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
 
 	proposedTxNote   string
 	proposedTxNoteMu sync.Mutex
+
+	log *logrus.Entry
 }
 
 // NewBaseAccount creates a new Account instance.
 func NewBaseAccount(config *AccountConfig, coin coin.Coin, log *logrus.Entry) *BaseAccount {
 	account := &BaseAccount{
 		config: config,
 		coin:   coin,
+		log:    log,
 	}
 	account.Synchronizer = synchronizer.NewSynchronizer(
 		func() { config.OnEvent(EventSyncStarted) },
@@ -131,20 +141,65 @@ func (account *BaseAccount) SetOffline(offline error) {
 // Initialize initializes the account. `accountIdentifier` is used as part of the filename of
 // account databases.
 func (account *BaseAccount) Initialize(accountIdentifier string) error {
-	notes, err := notes.LoadNotes(path.Join(
+	txNotes, err := notes.LoadNotes(path.Join(
 		account.config.NotesFolder,
 		fmt.Sprintf("%s.json", accountIdentifier),
 	))
 	if err != nil {
 		return err
 	}
-	account.notes = notes
-	return nil
-}
+	account.notes = []*notes.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 len(account.Config().SigningConfigurations) == 0 {
+		return nil
+	}
+	accountNumber, err := account.Config().SigningConfigurations[0].AccountNumber()
+	if err != nil {
+		return nil
+	}
+	if accountNumber != 0 {
+		// Up to v4.27.0, we only had one account per coin.
+		return nil
+	}
 
-// Notes implements Interface.
-func (account *BaseAccount) Notes() *notes.Notes {
-	return account.notes
+	legacyConfigurations := signing.ConvertToLegacyConfigurations(account.Config().SigningConfigurations)
+	var legacyAccountIdentifiers []string
+	switch account.coin.Code() {
+	case coin.CodeBTC, coin.CodeTBTC, coin.CodeLTC, coin.CodeTLTC:
+		legacyAccountIdentifiers = []string{fmt.Sprintf("account-%s-%s", legacyConfigurations.Hash(), account.coin.Code())}
+		// Also consider split accounts:
+		for _, cfg := range account.Config().SigningConfigurations {
+			legacyConfigurations := signing.ConvertToLegacyConfigurations(signing.Configurations{cfg})
+			legacyAccountIdentifier := fmt.Sprintf("account-%s-%s-%s", legacyConfigurations.Hash(), account.coin.Code(), cfg.ScriptType())
+			legacyAccountIdentifiers = append(
+				legacyAccountIdentifiers,
+				legacyAccountIdentifier,
+			)
+		}
+	default:
+		legacyAccountIdentifiers = []string{
+			fmt.Sprintf("account-%s-%s", legacyConfigurations[0].Hash(), account.coin.Code()),
+		}
+	}
+	for _, identifier := range legacyAccountIdentifiers {
+		notesFile := path.Join(account.config.NotesFolder, fmt.Sprintf("%s.json", identifier))
+		if _, err := os.Stat(notesFile); os.IsNotExist(err) {
+			continue // skip nonexistent legacy notes file
+		}
+
+		legacyNotes, err := notes.LoadNotes(path.Join(
+			account.config.NotesFolder,
+			fmt.Sprintf("%s.json", identifier),
+		))
+		if err != nil {
+			return err
+		}
+		account.notes = append(account.notes, legacyNotes)
+	}
+
+	return nil
 }
 
 // ProposeTxNote implements accounts.Account.
@@ -168,14 +223,33 @@ func (account *BaseAccount) GetAndClearProposedTxNote() string {
 
 // SetTxNote implements accounts.Account.
 func (account *BaseAccount) SetTxNote(txID string, note string) error {
-	if err := account.notes.SetTxNote(txID, note); err != nil {
+	// The notes slice is guaranteed to have at least one element by BaseAccount.Initialize.
+	if err := account.notes[0].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(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 ""
+}
+
 // ExportCSV implements accounts.Account.
 func (account *BaseAccount) ExportCSV(w io.Writer, transactions []*TransactionData) error {
 	writer := csv.NewWriter(w)
@@ -221,7 +295,7 @@ func (account *BaseAccount) ExportCSV(w io.Writer, transactions []*TransactionDa
 				feeString,
 				addressAndAmount.Address,
 				transaction.TxID,
-				account.Notes().TxNote(transaction.InternalID),
+				account.TxNote(transaction.InternalID),
 			})
 			if err != nil {
 				return errp.WithStack(err)

backend/accounts/baseaccount_test.go

@@ -17,11 +17,15 @@ package accounts
 import (
 	"bytes"
 	"errors"
+	"io/ioutil"
+	"path"
 	"testing"
 	"time"
 
+	"github.com/btcsuite/btcutil/hdkeychain"
 	"github.com/digitalbitbox/bitbox-wallet-app/backend/coins/coin"
 	"github.com/digitalbitbox/bitbox-wallet-app/backend/coins/coin/mocks"
+	"github.com/digitalbitbox/bitbox-wallet-app/backend/signing"
 	"github.com/digitalbitbox/bitbox-wallet-app/util/logging"
 	"github.com/digitalbitbox/bitbox-wallet-app/util/test"
 	"github.com/stretchr/testify/require"
@@ -38,20 +42,65 @@ func TestBaseAccount(t *testing.T) {
 		}
 		panic("can't reach")
 	}
+
+	derivationPath, err := signing.NewAbsoluteKeypath("m/84'/1'/0'")
+	require.NoError(t, err)
+	const xpub = "tpubDCxoQyC5JaGydxN3yprM6sgqgu65LruN3JBm1fnSmGxXR3AcuNwr" +
+		"E7J2CVaCvuLPJtJNySjshNsYbR96Y7yfEdcywYqWubzUQLVGh2b4mF9"
+	extendedPublicKey, err := hdkeychain.NewKeyFromString(xpub)
+	require.NoError(t, err)
+
 	const accountIdentifier = "test-account-identifier"
 	cfg := &AccountConfig{
-		Code:                  "test",
-		Name:                  "Test",
-		DBFolder:              test.TstTempDir("baseaccount_test_dbfolder"),
-		NotesFolder:           test.TstTempDir("baseaccount_test_notesfolder"),
-		Keystores:             nil,
-		OnEvent:               func(event Event) { events <- event },
-		RateUpdater:           nil,
-		SigningConfigurations: nil,
-		GetNotifier:           nil,
+		Code:        "test",
+		Name:        "Test",
+		DBFolder:    test.TstTempDir("baseaccount_test_dbfolder"),
+		NotesFolder: test.TstTempDir("baseaccount_test_notesfolder"),
+		Keystores:   nil,
+		OnEvent:     func(event Event) { events <- event },
+		RateUpdater: nil,
+		SigningConfigurations: signing.Configurations{
+			signing.NewBitcoinConfiguration(signing.ScriptTypeP2PKH, []byte{1, 2, 3, 4}, derivationPath, extendedPublicKey),
+			signing.NewBitcoinConfiguration(signing.ScriptTypeP2WPKH, []byte{1, 2, 3, 4}, derivationPath, extendedPublicKey),
+			signing.NewBitcoinConfiguration(signing.ScriptTypeP2WPKHP2SH, []byte{1, 2, 3, 4}, derivationPath, extendedPublicKey),
+		},
+		GetNotifier: nil,
 	}
+	// The long ID in the filename is the legacy hash of the configurations above (unified and split).
+	// This tests notes migration from v4.27.0 to v4.28.0.
+	require.NoError(t,
+		ioutil.WriteFile(
+			path.Join(cfg.NotesFolder, "account-54b4597c3a5c48177ef2b12c97e0cb30b6fef0b431e7821675b17704c330ce5d-tbtc.json"),
+			[]byte(`{"transactions": { "legacy-1": "legacy note in unified account" }}`),
+			0666,
+		),
+	)
+	require.NoError(t,
+		ioutil.WriteFile(
+			path.Join(cfg.NotesFolder, "account-989b84ec36f0b84e7926f9f5715e4b59a0592993b1fa4b70836addbcb0cb6e09-tbtc-p2pkh.json"),
+			[]byte(`{"transactions": { "legacy-2": "legacy note in split account, p2pkh" }}`),
+			0666,
+		),
+	)
+	require.NoError(t,
+		ioutil.WriteFile(
+			path.Join(cfg.NotesFolder, "account-e60b99507ba983d522f15932dbe1214e99de13c56e2bea75ed9c285f7c013117-tbtc-p2wpkh.json"),
+			[]byte(`{"transactions": { "legacy-3": "legacy note in split account, p2wpkh" }}`),
+			0666,
+		),
+	)
+	require.NoError(t,
+		ioutil.WriteFile(
+			path.Join(cfg.NotesFolder, "account-9e779e0d49e77236f0769e0bab2fd656958d3fd023dce2388525ee66fead88bb-tbtc-p2wpkh-p2sh.json"),
+			[]byte(`{"transactions": { "legacy-4": "legacy note in split account, p2wpkh-p2sh" }}`),
+			0666,
+		),
+	)
 
 	mockCoin := &mocks.CoinMock{
+		CodeFunc: func() coin.Code {
+			return coin.CodeTBTC
+		},
 		SmallestUnitFunc: func() string {
 			return "satoshi"
 		},
@@ -103,12 +152,26 @@ func TestBaseAccount(t *testing.T) {
 		// Was cleared by the previous call.
 		require.Equal(t, "", account.GetAndClearProposedTxNote())
 
-		notes := account.Notes()
-		require.NotNil(t, notes)
-
 		require.NoError(t, account.SetTxNote("test-tx-id", "another test note"))
 		require.Equal(t, EventStatusChanged, checkEvent())
-		require.Equal(t, "another test note", notes.TxNote("test-tx-id"))
+		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, "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 := ioutil.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 = ioutil.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) {
@@ -122,7 +185,7 @@ func TestBaseAccount(t *testing.T) {
 
 		require.Equal(t, header, export(nil))
 
-		require.NoError(t, account.notes.SetTxNote("some-internal-tx-id", "some note, with a comma"))
+		require.NoError(t, account.SetTxNote("some-internal-tx-id", "some note, with a comma"))
 		fee := coin.NewAmountFromInt64(101)
 		timestamp := time.Date(2020, 2, 30, 16, 44, 20, 0, time.UTC)
 		require.Equal(t,

backend/coins/btc/handlers/handlers.go

@@ -175,7 +175,7 @@ func (handlers *Handlers) getAccountTransactions(_ *http.Request) (interface{},
 			Fee:       feeString,
 			Time:      formattedTime,
 			Addresses: addresses,
-			Note:      handlers.account.Notes().TxNote(txInfo.InternalID),
+			Note:      handlers.account.TxNote(txInfo.InternalID),
 		}
 		switch handlers.account.Coin().(type) {
 		case *btc.Coin: