Merge branch 'fixnotes'

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: