From e9a2f19c17237017bae1fdd64be7f46624d65996 Mon Sep 17 00:00:00 2001 From: Ivan Belyakov Date: Tue, 26 Mar 2024 13:05:49 +0100 Subject: [PATCH] fix(wallet): cleanup multi_transactions table on account removed Updates #4937 --- services/wallet/history/service_test.go | 5 +- services/wallet/keycard_pairings_test.go | 6 +- services/wallet/transfer/controller.go | 5 + services/wallet/transfer/controller_test.go | 99 ++++++++++++++++--- .../transaction_manager_multitransaction.go | 64 ++++++++++++ 5 files changed, 164 insertions(+), 15 deletions(-) diff --git a/services/wallet/history/service_test.go b/services/wallet/history/service_test.go index 4e07a7209..b9637b752 100644 --- a/services/wallet/history/service_test.go +++ b/services/wallet/history/service_test.go @@ -418,13 +418,14 @@ func Test_removeBalanceHistoryOnEventAccountRemoved(t *testing.T) { Accounts: []common.Address{address}, }) - require.NoError(t, utils.Eventually(func() error { + err := utils.Eventually(func() error { entries, err := database.getNewerThan(&assetIdentity{1, []common.Address{address}, "ETH"}, 0) if err == nil && len(entries) == 0 { return nil } return errors.New("data is not removed") - }, 100*time.Millisecond, 10*time.Millisecond)) + }, 100*time.Millisecond, 10*time.Millisecond) + require.NoError(t, err) }() group.Wait() diff --git a/services/wallet/keycard_pairings_test.go b/services/wallet/keycard_pairings_test.go index 84a356761..883af087e 100644 --- a/services/wallet/keycard_pairings_test.go +++ b/services/wallet/keycard_pairings_test.go @@ -6,6 +6,8 @@ import ( "github.com/stretchr/testify/require" + "github.com/ethereum/go-ethereum/event" + "github.com/status-im/status-go/appdatabase" "github.com/status-im/status-go/multiaccounts/accounts" "github.com/status-im/status-go/params" @@ -25,7 +27,9 @@ func TestKeycardPairingsFile(t *testing.T) { db, err := helpers.SetupTestMemorySQLDB(walletdatabase.DbInitializer{}) require.NoError(t, err) - service := NewService(db, accountsDb, appDB, &rpc.Client{NetworkManager: network.NewManager(db)}, nil, nil, nil, nil, ¶ms.NodeConfig{}, nil, nil, nil, nil, nil) + accountFeed := &event.Feed{} + + service := NewService(db, accountsDb, appDB, &rpc.Client{NetworkManager: network.NewManager(db)}, accountFeed, nil, nil, nil, ¶ms.NodeConfig{}, nil, nil, nil, nil, nil) data, err := service.KeycardPairings().GetPairingsJSONFileContent() require.NoError(t, err) diff --git a/services/wallet/transfer/controller.go b/services/wallet/transfer/controller.go index 14d1051dc..e89f60386 100644 --- a/services/wallet/transfer/controller.go +++ b/services/wallet/transfer/controller.go @@ -266,6 +266,11 @@ func (c *Controller) cleanUpRemovedAccount(address common.Address) { if err != nil { log.Error("Failed to delete blocks ranges sequential", "error", err) } + + err = c.transactionManager.removeMultiTransactionByAddress(address) + if err != nil { + log.Error("Failed to delete multitransactions", "error", err) + } } func (c *Controller) cleanupAccountsLeftovers() error { diff --git a/services/wallet/transfer/controller_test.go b/services/wallet/transfer/controller_test.go index 1eea23ce8..3274d90e3 100644 --- a/services/wallet/transfer/controller_test.go +++ b/services/wallet/transfer/controller_test.go @@ -1,19 +1,23 @@ package transfer import ( + "context" "math/big" + "sync" "testing" "time" "github.com/stretchr/testify/require" "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/common/hexutil" "github.com/ethereum/go-ethereum/event" "github.com/status-im/status-go/appdatabase" "github.com/status-im/status-go/eth-node/types" "github.com/status-im/status-go/multiaccounts/accounts" "github.com/status-im/status-go/services/accounts/accountsevent" "github.com/status-im/status-go/services/wallet/blockchainstate" + wallet_common "github.com/status-im/status-go/services/wallet/common" "github.com/status-im/status-go/t/helpers" "github.com/status-im/status-go/walletdatabase" ) @@ -31,16 +35,17 @@ func TestController_watchAccountsChanges(t *testing.T) { accountFeed := &event.Feed{} bcstate := blockchainstate.NewBlockChainState() + transactionManager := NewTransactionManager(walletDB, nil, nil, nil, accountsDB, nil, nil) c := NewTransferController( walletDB, accountsDB, nil, // rpcClient accountFeed, - nil, // transferFeed - nil, // transactionManager - nil, // pendingTxManager - nil, // tokenManager - nil, // balanceCacher + nil, // transferFeed + transactionManager, // transactionManager + nil, // pendingTxManager + nil, // tokenManager + nil, // balanceCacher bcstate, ) @@ -80,11 +85,73 @@ func TestController_watchAccountsChanges(t *testing.T) { require.NoError(t, err) require.NotNil(t, ranges) + // Insert multitransactions + // Save address to accounts DB which transactions we want to preserve + counterparty := common.Address{0x1} + err = accountsDB.SaveOrUpdateAccounts([]*accounts.Account{ + {Address: types.Address(counterparty), Chat: false, Wallet: true}, + }, false) + require.NoError(t, err) + + // Self multi transaction + midSelf, err := transactionManager.InsertMultiTransaction(&MultiTransaction{ + FromAddress: address, + ToAddress: address, + FromAsset: "ETH", + ToAsset: "DAI", + FromAmount: &hexutil.Big{}, + ToAmount: &hexutil.Big{}, + Timestamp: 1, + }) + + require.NoError(t, err) + mtxs, err := transactionManager.GetMultiTransactions(context.Background(), []wallet_common.MultiTransactionIDType{midSelf}) + require.NoError(t, err) + require.Len(t, mtxs, 1) + + // Send multi transaction + mt := &MultiTransaction{ + FromAddress: address, + ToAddress: counterparty, + FromAsset: "ETH", + ToAsset: "DAI", + FromAmount: &hexutil.Big{}, + ToAmount: &hexutil.Big{}, + Timestamp: 2, + } + mid, err := transactionManager.InsertMultiTransaction(mt) + + require.NoError(t, err) + mtxs, err = transactionManager.GetMultiTransactions(context.Background(), []wallet_common.MultiTransactionIDType{midSelf, mid}) + require.NoError(t, err) + require.Len(t, mtxs, 2) + + // Another Send multi-transaction where sender and receiver are inverted (both accounts are in accounts DB) + midReverse, err := transactionManager.InsertMultiTransaction(&MultiTransaction{ + FromAddress: mt.ToAddress, + ToAddress: mt.FromAddress, + FromAsset: mt.FromAsset, + ToAsset: mt.ToAsset, + FromAmount: mt.FromAmount, + ToAmount: mt.ToAmount, + Timestamp: mt.Timestamp + 1, + }) + + require.NoError(t, err) + mtxs, err = transactionManager.GetMultiTransactions(context.Background(), []wallet_common.MultiTransactionIDType{midSelf, mid, midReverse}) + require.NoError(t, err) + require.Len(t, mtxs, 3) + + // Start watching accounts + wg := sync.WaitGroup{} + wg.Add(1) c.accWatcher = accountsevent.NewWatcher(c.accountsDB, c.accountFeed, func(changedAddresses []common.Address, eventType accountsevent.EventType, currentAddresses []common.Address) { c.onAccountsChanged(changedAddresses, eventType, currentAddresses, []uint64{chainID}) // Quit channel event handler before destroying the channel go func() { + defer wg.Done() + time.Sleep(1 * time.Millisecond) // Wait for DB to be cleaned up c.accWatcher.Stop() @@ -107,6 +174,11 @@ func TestController_watchAccountsChanges(t *testing.T) { require.Nil(t, ranges.tokens.FirstKnown) require.Nil(t, ranges.tokens.LastKnown) require.Nil(t, ranges.tokens.Start) + + mtxs, err := transactionManager.GetMultiTransactions(context.Background(), []wallet_common.MultiTransactionIDType{mid, midSelf, midReverse}) + require.NoError(t, err) + require.Len(t, mtxs, 1) + require.Equal(t, midReverse, mtxs[0].ID) }() }) c.startAccountWatcher([]uint64{chainID}) @@ -122,6 +194,8 @@ func TestController_watchAccountsChanges(t *testing.T) { Accounts: []common.Address{address}, }) }() + + wg.Wait() } func TestController_cleanupAccountLeftovers(t *testing.T) { @@ -146,17 +220,18 @@ func TestController_cleanupAccountLeftovers(t *testing.T) { require.NoError(t, err) require.Len(t, storedAccs, 1) + transactionManager := NewTransactionManager(walletDB, nil, nil, nil, accountsDB, nil, nil) bcstate := blockchainstate.NewBlockChainState() c := NewTransferController( walletDB, accountsDB, - nil, // rpcClient - nil, // accountFeed - nil, // transferFeed - nil, // transactionManager - nil, // pendingTxManager - nil, // tokenManager - nil, // balanceCacher + nil, // rpcClient + nil, // accountFeed + nil, // transferFeed + transactionManager, // transactionManager + nil, // pendingTxManager + nil, // tokenManager + nil, // balanceCacher bcstate, ) chainID := uint64(777) diff --git a/services/wallet/transfer/transaction_manager_multitransaction.go b/services/wallet/transfer/transaction_manager_multitransaction.go index ab7cbb7d6..4ef99f30d 100644 --- a/services/wallet/transfer/transaction_manager_multitransaction.go +++ b/services/wallet/transfer/transaction_manager_multitransaction.go @@ -458,3 +458,67 @@ func (tm *TransactionManager) getVerifiedWalletAccount(address, password string) AccountKey: key, }, nil } + +func (tm *TransactionManager) removeMultiTransactionByAddress(address common.Address) error { + // We must not remove those transactions, where from_address and to_address are different and both are stored in accounts DB + // and one of them is equal to the address, as we want to keep the records for the other address + // That is why we don't use cascade delete here with references to transfers table, as we might have 2 records in multi_transactions + // for the same transaction, one for each address + + stmt, err := tm.db.Prepare(`SELECT rowid, from_address, to_address + FROM multi_transactions + WHERE from_address=? OR to_address=?`) + if err != nil { + return err + } + + rows, err := stmt.Query(address, address) + if err != nil { + return err + } + defer rows.Close() + + rowIDs := make([]int, 0) + rowID, fromAddress, toAddress := 0, common.Address{}, common.Address{} + for rows.Next() { + err = rows.Scan(&rowID, &fromAddress, &toAddress) + if err != nil { + log.Error("Failed to scan row", "error", err) + continue + } + + // Remove self transactions as well, leave only those where we have the counterparty in accounts DB + if fromAddress != toAddress { + // If both addresses are stored in accounts DB, we don't remove the record + var addressToCheck common.Address + if fromAddress == address { + addressToCheck = toAddress + } else { + addressToCheck = fromAddress + } + counterpartyExists, err := tm.accountsDB.AddressExists(types.Address(addressToCheck)) + if err != nil { + log.Error("Failed to query accounts db for a given address", "address", address, "error", err) + continue + } + + // Skip removal if counterparty is in accounts DB and removed address is not sender + if counterpartyExists && address != fromAddress { + continue + } + } + + rowIDs = append(rowIDs, rowID) + } + + if len(rowIDs) > 0 { + for _, rowID := range rowIDs { + _, err = tm.db.Exec(`DELETE FROM multi_transactions WHERE rowid=?`, rowID) + if err != nil { + log.Error("Failed to remove multitransaction", "rowid", rowID, "error", err) + } + } + } + + return err +}