fix: fixed accounts positions in case of account deletion

multiaccounts/accounts/database.go

@@ -283,8 +283,9 @@ func GetAccountTypeForKeypairType(kpType KeypairType) AccountType {
 	}
 }
 
-func (db *Database) processKeypairs(rows *sql.Rows) ([]*Keypair, error) {
+func (db *Database) processRows(rows *sql.Rows) ([]*Keypair, []*Account, error) {
 	keypairMap := make(map[string]*Keypair)
+	allAccounts := []*Account{}
 
 	var (
 		kpKeyUID                  sql.NullString
@@ -321,7 +322,7 @@ func (db *Database) processKeypairs(rows *sql.Rows) ([]*Keypair, error) {
 			&accAddress, &accKeyUID, &pubkey, &accPath, &accName, &accColorID, &accEmoji,
 			&accWallet, &accChat, &accHidden, &accOperable, &accClock, &accCreatedAt, &accPosition)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 
 		// check keypair fields
@@ -392,14 +393,17 @@ func (db *Database) processKeypairs(rows *sql.Rows) ([]*Keypair, error) {
 		}
 		acc.Type = GetAccountTypeForKeypairType(kp.Type)
 
+		if kp.KeyUID != "" {
 			if _, ok := keypairMap[kp.KeyUID]; !ok {
 				keypairMap[kp.KeyUID] = kp
 			}
 			keypairMap[kp.KeyUID].Accounts = append(keypairMap[kp.KeyUID].Accounts, acc)
 		}
+		allAccounts = append(allAccounts, acc)
+	}
 
 	if err := rows.Err(); err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	// Convert map to list
@@ -408,7 +412,7 @@ func (db *Database) processKeypairs(rows *sql.Rows) ([]*Keypair, error) {
 		keypairs = append(keypairs, keypair)
 	}
 
-	return keypairs, nil
+	return keypairs, allAccounts, nil
 }
 
 // If `keyUID` is passed only keypairs which match the passed `keyUID` will be returned, if `keyUID` is empty, all keypairs will be returned.
@@ -481,7 +485,7 @@ func (db *Database) getKeypairs(tx *sql.Tx, keyUID string) ([]*Keypair, error) {
 
 	defer rows.Close()
 
-	keypairs, err := db.processKeypairs(rows)
+	keypairs, _, err := db.processRows(rows)
 	if err != nil {
 		return nil, err
 	}
@@ -577,14 +581,10 @@ func (db *Database) getAccounts(tx *sql.Tx, address types.Address) ([]*Account,
 	}
 
 	defer rows.Close()
-	keypairs, err := db.processKeypairs(rows)
+	_, allAccounts, err := db.processRows(rows)
 	if err != nil {
 		return nil, err
 	}
-	allAccounts := []*Account{}
-	for _, kp := range keypairs {
-		allAccounts = append(allAccounts, kp.Accounts...)
-	}
 
 	return allAccounts, nil
 }
@@ -832,6 +832,12 @@ func (db *Database) saveOrUpdateAccounts(tx *sql.Tx, accounts []*Account, update
 			return err
 		}
 
+		// Update positions change clock when adding new/updating account
+		err = db.setClockOfLastAccountsPositionChange(tx, acc.Clock)
+		if err != nil {
+			return err
+		}
+
 		// Update keypair clock if any but the watch only account has changed.
 		if relatedKeypair != nil && updateKeypairClock {
 			err = db.updateKeypairClock(tx, acc.KeyUID, acc.Clock)
@@ -1151,6 +1157,37 @@ func (db *Database) GetClockOfLastAccountsPositionChange() (result uint64, err e
 	return result, err
 }
 
+// Updates positions of accounts respecting current order.
+func (db *Database) ResolveAccountsPositions(clock uint64) (err error) {
+	tx, err := db.db.Begin()
+	defer func() {
+		if err == nil {
+			err = tx.Commit()
+			return
+		}
+		_ = tx.Rollback()
+	}()
+
+	// returns all accounts ordered by position
+	dbAccounts, err := db.getAccounts(tx, types.Address{})
+	if err != nil {
+		return err
+	}
+
+	// starting from -1, cause `getAccounts` returns chat account as well
+	for i := 0; i < len(dbAccounts); i++ {
+		expectedPosition := int64(i - 1)
+		if dbAccounts[i].Position != expectedPosition {
+			_, err = tx.Exec("UPDATE keypairs_accounts SET position = ? WHERE address = ?", expectedPosition, dbAccounts[i].Address)
+			if err != nil {
+				return err
+			}
+		}
+	}
+
+	return db.setClockOfLastAccountsPositionChange(tx, clock)
+}
+
 // Sets positions for passed accounts.
 func (db *Database) SetWalletAccountsPositions(accounts []*Account, clock uint64) (err error) {
 	if len(accounts) == 0 {

multiaccounts/accounts/test_helper.go

@@ -327,6 +327,10 @@ func SameAccounts(expected, real *Account) bool {
 		expected.Removed == real.Removed
 }
 
+func SameAccountsIncludingPosition(expected, real *Account) bool {
+	return SameAccounts(expected, real) && expected.Position == real.Position
+}
+
 func SameAccountsWithDifferentOperable(expected, real *Account, expectedOperableValue AccountOperable) bool {
 	return SameAccounts(expected, real) && real.Operable == expectedOperableValue
 }

protocol/messenger.go

@@ -2601,6 +2601,11 @@ func (m *Messenger) SyncDevices(ctx context.Context, ensName, photoPath string,
 		return err
 	}
 
+	err = m.syncAccountsPositions(rawMessageHandler)
+	if err != nil {
+		return err
+	}
+
 	return m.syncSocialLinks(context.Background(), rawMessageHandler)
 }
 

protocol/messenger_handler.go

@@ -3102,6 +3102,10 @@ func (m *Messenger) handleSyncWatchOnlyAccount(message *protobuf.SyncAccount) (*
 
 		if message.Removed {
 			err = m.settings.DeleteAccount(accAddress, message.Clock)
+			if err != nil {
+				return nil, err
+			}
+			err = m.settings.ResolveAccountsPositions(message.Clock)
 			dbAccount.Removed = true
 			return dbAccount, err
 		}
@@ -3131,7 +3135,9 @@ func (m *Messenger) handleSyncAccountsPositions(message *protobuf.SyncAccountsPo
 		return nil, err
 	}
 
-	if message.Clock <= dbLastUpdate {
+	// Since adding new account updates `ClockOfLastAccountsPositionChange` we should handle account order changes
+	// even they are with the same clock, that ensures the correct order in case of syncing devices.
+	if message.Clock < dbLastUpdate {
 		return nil, ErrTryingToApplyOldWalletAccountsOrder
 	}
 
@@ -3230,14 +3236,25 @@ func (m *Messenger) handleSyncKeypair(message *protobuf.SyncKeypair) (*accounts.
 
 	// if entire keypair was removed, there is no point to continue
 	if kp.Removed {
+		err = m.settings.ResolveAccountsPositions(message.Clock)
+		if err != nil {
+			return nil, err
+		}
 		return kp, nil
 	}
 
+	// save keypair first
 	err = m.settings.SaveOrUpdateKeypair(kp)
 	if err != nil {
 		return nil, err
 	}
 
+	// then resolve accounts positions, cause some accounts might be removed
+	err = m.settings.ResolveAccountsPositions(message.Clock)
+	if err != nil {
+		return nil, err
+	}
+
 	for _, sKc := range message.Keycards {
 		kc := accounts.Keycard{}
 		kc.FromSyncKeycard(sKc)
@@ -3248,7 +3265,12 @@ func (m *Messenger) handleSyncKeypair(message *protobuf.SyncKeypair) (*accounts.
 		kp.Keycards = append(kp.Keycards, &kc)
 	}
 
-	return kp, nil
+	// getting keypair form the db, cause keypair related accounts positions might be changed
+	dbKeypair, err = m.settings.GetKeypairByKeyUID(message.KeyUid)
+	if err != nil {
+		return nil, err
+	}
+	return dbKeypair, nil
 }
 
 func (m *Messenger) HandleSyncAccountsPositions(state *ReceivedMessageState, message protobuf.SyncAccountsPositions) error {

protocol/messenger_sync_wallets_test.go

@@ -362,7 +362,7 @@ func (s *MessengerSyncWalletSuite) TestSyncWalletAccountsReorder() {
 	s.Require().NoError(err)
 	s.Require().Equal(len(woAccounts), len(dbAccounts)-1)
 	for i := 0; i < len(woAccounts); i++ {
-		s.Require().True(accounts.SameAccounts(woAccounts[i], dbAccounts[i+1]))
+		s.Require().True(accounts.SameAccountsIncludingPosition(woAccounts[i], dbAccounts[i+1]))
 	}
 
 	// Sync between devices is triggered automatically
@@ -384,7 +384,7 @@ func (s *MessengerSyncWalletSuite) TestSyncWalletAccountsReorder() {
 	s.Require().NoError(err)
 	s.Require().Equal(len(woAccounts), len(dbAccounts)-1)
 	for i := 0; i < len(woAccounts); i++ {
-		s.Require().True(accounts.SameAccounts(woAccounts[i], dbAccounts[i+1]))
+		s.Require().True(accounts.SameAccountsIncludingPosition(woAccounts[i], dbAccounts[i+1]))
 	}
 
 	// compare times
@@ -412,7 +412,7 @@ func (s *MessengerSyncWalletSuite) TestSyncWalletAccountsReorder() {
 	s.Require().NoError(err)
 	s.Require().Equal(len(woAccounts), len(dbAccounts)-1)
 	for i := 0; i < len(woAccounts); i++ {
-		s.Require().True(accounts.SameAccounts(woAccounts[i], dbAccounts[i+1]))
+		s.Require().True(accounts.SameAccountsIncludingPosition(woAccounts[i], dbAccounts[i+1]))
 	}
 
 	// Sync between devices is triggered automatically
@@ -434,7 +434,7 @@ func (s *MessengerSyncWalletSuite) TestSyncWalletAccountsReorder() {
 	s.Require().NoError(err)
 	s.Require().Equal(len(woAccounts), len(dbAccounts)-1)
 	for i := 0; i < len(woAccounts); i++ {
-		s.Require().True(accounts.SameAccounts(woAccounts[i], dbAccounts[i+1]))
+		s.Require().True(accounts.SameAccountsIncludingPosition(woAccounts[i], dbAccounts[i+1]))
 	}
 
 	// compare times
@@ -444,3 +444,178 @@ func (s *MessengerSyncWalletSuite) TestSyncWalletAccountsReorder() {
 	s.Require().NoError(err)
 	s.Require().Equal(dbClock, dbClockOtherDevice)
 }
+
+func (s *MessengerSyncWalletSuite) TestSyncWalletAccountOrderAfterDeletion() {
+	profileKp := accounts.GetProfileKeypairForTest(true, true, true)
+	// set clocks for accounts
+	profileKp.Clock = uint64(len(profileKp.Accounts) - 1)
+	i := -1
+	for _, acc := range profileKp.Accounts {
+		acc.Clock = uint64(i + 1)
+		acc.Position = int64(i)
+		acc.Operable = accounts.AccountNonOperable
+		i++
+	}
+
+	// Create a main account on alice
+	err := s.m.settings.SaveOrUpdateKeypair(profileKp)
+	s.Require().NoError(err, "profile keypair alice.settings.SaveOrUpdateKeypair")
+	// Store seed phrase keypair with accounts on alice's device
+	seedPhraseKp := accounts.GetSeedImportedKeypair1ForTest()
+	for _, acc := range seedPhraseKp.Accounts {
+		acc.Clock = uint64(i + 1)
+		acc.Position = int64(i)
+		acc.Operable = accounts.AccountNonOperable
+		i++
+	}
+	err = s.m.settings.SaveOrUpdateKeypair(seedPhraseKp)
+	s.Require().NoError(err, "seed phrase keypair alice.settings.SaveOrUpdateKeypair")
+	// Store private key keypair with accounts on alice's device
+	privKeyKp := accounts.GetPrivKeyImportedKeypairForTest()
+	for _, acc := range privKeyKp.Accounts {
+		acc.Clock = uint64(i + 1)
+		acc.Position = int64(i)
+		acc.Operable = accounts.AccountNonOperable
+		i++
+	}
+	err = s.m.settings.SaveOrUpdateKeypair(privKeyKp)
+	s.Require().NoError(err, "private key keypair alice.settings.SaveOrUpdateKeypair")
+	// Store watch only accounts on alice's device
+	woAccounts := accounts.GetWatchOnlyAccountsForTest()
+	for _, acc := range woAccounts {
+		acc.Clock = uint64(i + 1)
+		acc.Position = int64(i)
+		acc.Operable = accounts.AccountFullyOperable
+		i++
+	}
+	err = s.m.settings.SaveOrUpdateAccounts(woAccounts, false)
+	s.Require().NoError(err)
+	// Check accounts
+	dbAccounts1, err := s.m.settings.GetAccounts()
+	s.Require().NoError(err)
+	totalNumOfAccounts := len(profileKp.Accounts) + len(seedPhraseKp.Accounts) + len(privKeyKp.Accounts) + len(woAccounts)
+	s.Require().Equal(totalNumOfAccounts, len(dbAccounts1))
+
+	// Create new device and add main account to
+	alicesOtherDevice, err := newMessengerWithKey(s.shh, s.m.identity, s.logger, nil)
+	s.Require().NoError(err)
+	// Store only chat and default wallet account on other device
+	profileKpOtherDevice := accounts.GetProfileKeypairForTest(true, true, false)
+	err = alicesOtherDevice.settings.SaveOrUpdateKeypair(profileKpOtherDevice)
+	s.Require().NoError(err, "profile keypair alicesOtherDevice.settings.SaveOrUpdateKeypair")
+
+	// Pair devices
+	im1 := &multidevice.InstallationMetadata{
+		Name:       "alice's-other-device",
+		DeviceType: "alice's-other-device-type",
+	}
+	err = alicesOtherDevice.SetInstallationMetadata(alicesOtherDevice.installationID, im1)
+	s.Require().NoError(err)
+	response, err := alicesOtherDevice.SendPairInstallation(context.Background(), nil)
+	s.Require().NoError(err)
+	s.Require().NotNil(response)
+	s.Require().Len(response.Chats(), 1)
+	s.Require().False(response.Chats()[0].Active)
+
+	// Wait for the message to reach its destination
+	response, err = WaitOnMessengerResponse(
+		s.m,
+		func(r *MessengerResponse) bool { return len(r.Installations) > 0 },
+		"installation not received",
+	)
+
+	s.Require().NoError(err)
+	actualInstallation := response.Installations[0]
+	s.Require().Equal(alicesOtherDevice.installationID, actualInstallation.ID)
+	s.Require().NotNil(actualInstallation.InstallationMetadata)
+	s.Require().Equal("alice's-other-device", actualInstallation.InstallationMetadata.Name)
+	s.Require().Equal("alice's-other-device-type", actualInstallation.InstallationMetadata.DeviceType)
+
+	err = s.m.EnableInstallation(alicesOtherDevice.installationID)
+	s.Require().NoError(err)
+
+	// Trigger's a sync between devices
+	err = s.m.SyncDevices(context.Background(), "ens-name", "profile-image", nil)
+	s.Require().NoError(err)
+
+	err = tt.RetryWithBackOff(func() error {
+		response, err := alicesOtherDevice.RetrieveAll()
+		if err != nil {
+			return err
+		}
+
+		if len(response.Keypairs) != 3 || // 3 keypairs (profile, seed, priv key)
+			len(response.WatchOnlyAccounts) != len(woAccounts) ||
+			len(response.AccountsPositions) != totalNumOfAccounts-1 /* we don't include chat account in position ordering*/ {
+			return errors.New("no sync wallet account received")
+		}
+		return nil
+	})
+	s.Require().NoError(err)
+
+	dbAccounts2, err := alicesOtherDevice.settings.GetAccounts()
+	s.Require().NoError(err)
+	s.Require().Equal(totalNumOfAccounts, len(dbAccounts2))
+
+	s.Require().True(haveSameElements(dbAccounts1, dbAccounts2, accounts.SameAccountsIncludingPosition))
+
+	// Delete keypair related account on alice's primary device
+	accToDelete := seedPhraseKp.Accounts[1]
+	err = s.m.DeleteAccount(accToDelete.Address)
+	s.Require().NoError(err, "delete account on alice primary device")
+
+	totalNumOfAccounts-- //one acc less
+
+	err = tt.RetryWithBackOff(func() error {
+		response, err := alicesOtherDevice.RetrieveAll()
+		if err != nil {
+			return err
+		}
+
+		if len(response.Keypairs) != 1 {
+			return errors.New("no sync keypairs received")
+		}
+		return nil
+	})
+	s.Require().NoError(err)
+
+	dbAccounts1, err = s.m.settings.GetAccounts()
+	s.Require().NoError(err)
+	s.Require().Equal(totalNumOfAccounts, len(dbAccounts1))
+
+	dbAccounts2, err = alicesOtherDevice.settings.GetAccounts()
+	s.Require().NoError(err)
+	s.Require().Equal(totalNumOfAccounts, len(dbAccounts2))
+
+	s.Require().True(haveSameElements(dbAccounts1, dbAccounts2, accounts.SameAccountsIncludingPosition))
+
+	// Delete watch only account on alice's primary device
+	accToDelete = woAccounts[1]
+	err = s.m.DeleteAccount(accToDelete.Address)
+	s.Require().NoError(err, "delete account on alice primary device")
+
+	totalNumOfAccounts-- //one acc less
+
+	err = tt.RetryWithBackOff(func() error {
+		response, err := alicesOtherDevice.RetrieveAll()
+		if err != nil {
+			return err
+		}
+
+		if len(response.WatchOnlyAccounts) != 1 {
+			return errors.New("no sync keypairs received")
+		}
+		return nil
+	})
+	s.Require().NoError(err)
+
+	dbAccounts1, err = s.m.settings.GetAccounts()
+	s.Require().NoError(err)
+	s.Require().Equal(totalNumOfAccounts, len(dbAccounts1))
+
+	dbAccounts2, err = alicesOtherDevice.settings.GetAccounts()
+	s.Require().NoError(err)
+	s.Require().Equal(totalNumOfAccounts, len(dbAccounts2))
+
+	s.Require().True(haveSameElements(dbAccounts1, dbAccounts2, accounts.SameAccountsIncludingPosition))
+}

protocol/messenger_wallet.go

@@ -237,7 +237,20 @@ func (m *Messenger) DeleteAccount(address types.Address) error {
 		return err
 	}
 
-	return m.resolveAndSyncKeypairOrJustWalletAccount(acc.KeyUID, acc.Address, clock, m.dispatchMessage)
+	err = m.resolveAndSyncKeypairOrJustWalletAccount(acc.KeyUID, acc.Address, clock, m.dispatchMessage)
+	if err != nil {
+		return err
+	}
+
+	// In case when user deletes an account, we need to send sync message after an account gets deleted,
+	// and then (after that) update the positions of other accoutns. That's needed to handle properly
+	// accounts order on the paired devices.
+	err = m.settings.ResolveAccountsPositions(clock)
+	if err != nil {
+		return err
+	}
+	// Since some keypairs may be received out of expected order, we're aligning that by sending accounts position sync msg.
+	return m.syncAccountsPositions(m.dispatchMessage)
 }
 
 func (m *Messenger) prepareSyncAccountMessage(acc *accounts.Account) *protobuf.SyncAccount {