Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IOS-6610 add a mechanism for automatic indexation of wallets #3322

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Tangem/App/Services/Common/AppSettings.swift
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ class AppSettings {
@AppStorageCompat(StorageType.forcedDemoCardId)
var forcedDemoCardId: String? = nil

@AppStorageCompat(StorageType.didMigrateUserWalletNames)
var didMigrateUserWalletNames: Bool = false

static let shared: AppSettings = .init()

private init() {}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
//
// UserWalletNameIndexationHelper.swift
// Tangem
//
// Created by Andrey Chukavin on 17.05.2024.
// Copyright © 2024 Tangem AG. All rights reserved.
//

import Foundation

class UserWalletNameIndexationHelper {
private var indexesByNameTemplate: [String: [Int]] = [:]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indexes -> indices

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Что не так с indexes?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Чаще используется форма indices, в том числе Apple, это не крит в этой задаче

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я бы предложил оставить как есть чтобы не переусложнять

private let nameComponentsRegex = try! NSRegularExpression(pattern: "^(.+)(\\s+\\d+)$")

init(mode: Mode, names: [String]) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Давай сделаем утилиту stateless, без mode. Просто набор методов и немного подрефачим для простоты

публичные методы

  • миграция
    Миграция пусть работает сразу с моделями кошельков, тогда эта логика переедет из репозитория сюда и позже будет удалена.
  1. получили словари с занятыми индексами распарсив кошельки
  2. пошли в цикле по кошелькам, ищем пустые,
  • находим ближайший свободный индекс
  • конструируем имя и обновляем у кошелька
  • обновляем локальный словарь индексов (может сет заюзать?)
  • продолжаем, пока кошельки не кончатся
  1. возвращаем новые кошельки, если была миграция. Если не было, возвращаем nil или пустой массив, это хорошая оптимизация
  • поиск нового имени
  1. получили словари с занятыми индексами распарсив кошельки
  2. взяли ближайший свободный
  3. сконструировали имя и вернули

Приватные

  • Парсинг кошельков
  • поиск следующего индекса
  • конструктор имени

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ради простоты и тестов не хочется передавать сюда модели, поэтому сделал на строках. Передавать массив строк, получать его обратно и задавать имена массиву моделей выглядело ненадежно, поэтому сделал на скалярах

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

это комментарий не к тестам же

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Мой тоже

for name in names {
guard let nameComponents = nameComponents(from: name) else {
if mode == .newName {
addIndex(1, for: name)
}
continue
}

let indexesByNameTemplate = indexesByNameTemplate[nameComponents.template] ?? []
if !indexesByNameTemplate.contains(nameComponents.index) {
addIndex(nameComponents.index, for: nameComponents.template)
}
}
}

func suggestedName(_ rawName: String) -> String {
if let _ = nameComponents(from: rawName) {
return rawName
}

let nameTemplate = rawName.trimmingCharacters(in: .whitespaces)
let nameIndex = nextIndex(for: nameTemplate)

addIndex(nameIndex, for: nameTemplate)

if nameIndex == 1 {
return nameTemplate
} else {
return "\(nameTemplate) \(nameIndex)"
}
}

private func addIndex(_ index: Int, for nameTemplate: String) {
let newIndexes = (indexesByNameTemplate[nameTemplate] ?? []) + [index]
indexesByNameTemplate[nameTemplate] = newIndexes.sorted()
}

private func nextIndex(for nameTemplate: String) -> Int {
let indexes = indexesByNameTemplate[nameTemplate] ?? []

for i in 1 ... 100 {
if !indexes.contains(i) {
return i
}
}

let defaultIndex = indexes.count + 1
return defaultIndex
}

private func nameComponents(from rawName: String) -> NameComponents? {
let name = rawName.trimmingCharacters(in: .whitespaces)
let range = NSRange(location: 0, length: name.count)

guard
let match = nameComponentsRegex.matches(in: name, range: range).first,
match.numberOfRanges == 3,
let templateRange = Range(match.range(at: 1), in: name),
let indexRange = Range(match.range(at: 2), in: name),
let index = Int(String(name[indexRange]).trimmingCharacters(in: .whitespaces))
else {
return nil
}

let template = String(name[templateRange])
return NameComponents(template: template, index: index)
}
}

extension UserWalletNameIndexationHelper {
enum Mode {
case migration
case newName
}
}

private extension UserWalletNameIndexationHelper {
struct NameComponents {
let template: String
let index: Int
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ class CommonUserWalletRepository: UserWalletRepository {
let config = UserWalletConfigFactory(cardInfo).makeConfig()
Analytics.endLoggingCardScan()

cardInfo.name = config.cardName
cardInfo.name = suggestedUserWalletName(defaultName: config.cardName)

let userWalletModel = CommonUserWalletModel(cardInfo: cardInfo)
if let userWalletModel {
Expand Down Expand Up @@ -148,6 +148,13 @@ class CommonUserWalletRepository: UserWalletRepository {
.eraseToAnyPublisher()
}

private func suggestedUserWalletName(defaultName: String) -> String {
let otherNames = models.map(\.name)
let helper = UserWalletNameIndexationHelper(mode: .newName, names: otherNames)

return helper.suggestedName(defaultName)
}

func unlock(with method: UserWalletRepositoryUnlockMethod, completion: @escaping (UserWalletRepositoryResult?) -> Void) {
switch method {
case .biometry:
Expand Down Expand Up @@ -480,7 +487,8 @@ class CommonUserWalletRepository: UserWalletRepository {
}

private func loadModels() {
let savedUserWallets = savedUserWallets(withSensitiveData: true)
var savedUserWallets = savedUserWallets(withSensitiveData: true)
migrateNamesIfNeeded(&savedUserWallets)

models = savedUserWallets.map { userWalletStorageItem in
if let userWallet = CommonUserWalletModel(userWallet: userWalletStorageItem) {
Expand Down Expand Up @@ -511,6 +519,31 @@ class CommonUserWalletRepository: UserWalletRepository {
initializeServices(for: selectedModel)
}

private func migrateNamesIfNeeded(_ wallets: inout [StoredUserWallet]) {
if AppSettings.shared.didMigrateUserWalletNames {
return
}

AppSettings.shared.didMigrateUserWalletNames = true
tureck1y marked this conversation as resolved.
Show resolved Hide resolved

let oldNames = wallets.map(\.name)
let helper = UserWalletNameIndexationHelper(mode: .migration, names: oldNames)

var didChangeNames = true
for i in 0 ..< wallets.count {
let oldName = wallets[i].name
let newName = helper.suggestedName(oldName)
if newName != oldName {
wallets[i].name = newName
didChangeNames = true
}
}

if didChangeNames {
UserWalletRepositoryUtil().saveUserWallets(wallets)
}
}

private func sendEvent(_ event: UserWalletRepositoryEvent) {
eventSubject.send(event)
}
Expand Down
1 change: 1 addition & 0 deletions Tangem/Common/Storage/StorageType.swift
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,5 @@ enum StorageType: String {
case pendingBackups = "pending_backups"
case pendingBackupsCurrentID = "pending_backups_current_id"
case forcedDemoCardId = "forced_demo_card_id"
case didMigrateUserWalletNames = "did_migrate_user_wallet_names"
}
14 changes: 13 additions & 1 deletion Tangem/Modules/Main/MainViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ final class MainViewModel: ObservableObject {
private var pendingUserWalletIdsToUpdate: Set<UserWalletId> = []
private var pendingUserWalletModelsToAdd: [UserWalletModel] = []
private var shouldRecreatePagesAfterAddingPendingWalletModels = false
private var walletNameFieldValidator: AlertFieldValidator?

private var isLoggingOut = false

Expand Down Expand Up @@ -155,10 +156,21 @@ final class MainViewModel: ObservableObject {

guard let userWalletModel = userWalletRepository.selectedModel else { return }

let otherWalletNames = userWalletRepository.models.compactMap { model -> String? in
guard model.userWalletId != userWalletModel.userWalletId else { return nil }

return model.name
}

walletNameFieldValidator = AlertFieldValidator { input in
!otherWalletNames.contains(input)
}

let alert = AlertBuilder.makeAlertControllerWithTextField(
title: Localization.userWalletListRenamePopupTitle,
fieldPlaceholder: Localization.userWalletListRenamePopupPlaceholder,
fieldText: userWalletModel.name
fieldText: userWalletModel.name,
fieldValidator: walletNameFieldValidator
) { newName in
guard userWalletModel.name != newName else { return }

Expand Down
16 changes: 16 additions & 0 deletions TangemApp.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -849,6 +849,8 @@
DAAA3C722B3D813C0002DBB0 /* SendAddressService.swift in Sources */ = {isa = PBXBuildFile; fileRef = DAAA3C702B3D813C0002DBB0 /* SendAddressService.swift */; };
DAAA3C732B3D813C0002DBB0 /* SendAddressServiceFactory.swift in Sources */ = {isa = PBXBuildFile; fileRef = DAAA3C712B3D813C0002DBB0 /* SendAddressServiceFactory.swift */; };
DAAEC38D2BF31A8800184D7C /* SendFeatureProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = DAAEC38C2BF31A8800184D7C /* SendFeatureProvider.swift */; };
DAAF69482BFE1C0400179697 /* UserWalletNameIndexationHelper.swift in Sources */ = {isa = PBXBuildFile; fileRef = DAAF69462BFE1C0400179697 /* UserWalletNameIndexationHelper.swift */; };
DAAF694A2BFE1CFA00179697 /* UserWalletNameIndexationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = DAAF69492BFE1CFA00179697 /* UserWalletNameIndexationTests.swift */; };
DAB19D482B2C72A100DD08C4 /* AddCustomTokenCoordinator.swift in Sources */ = {isa = PBXBuildFile; fileRef = DAB19D432B2C72A100DD08C4 /* AddCustomTokenCoordinator.swift */; };
DAB19D492B2C72A100DD08C4 /* AddCustomTokenViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = DAB19D442B2C72A100DD08C4 /* AddCustomTokenViewModel.swift */; };
DAB19D4A2B2C72A100DD08C4 /* AddCustomTokenView.swift in Sources */ = {isa = PBXBuildFile; fileRef = DAB19D452B2C72A100DD08C4 /* AddCustomTokenView.swift */; };
Expand Down Expand Up @@ -2434,6 +2436,8 @@
DAAA3C702B3D813C0002DBB0 /* SendAddressService.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = SendAddressService.swift; sourceTree = "<group>"; };
DAAA3C712B3D813C0002DBB0 /* SendAddressServiceFactory.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = SendAddressServiceFactory.swift; sourceTree = "<group>"; };
DAAEC38C2BF31A8800184D7C /* SendFeatureProvider.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SendFeatureProvider.swift; sourceTree = "<group>"; };
DAAF69462BFE1C0400179697 /* UserWalletNameIndexationHelper.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = UserWalletNameIndexationHelper.swift; sourceTree = "<group>"; };
DAAF69492BFE1CFA00179697 /* UserWalletNameIndexationTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = UserWalletNameIndexationTests.swift; sourceTree = "<group>"; };
DAB19D432B2C72A100DD08C4 /* AddCustomTokenCoordinator.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = AddCustomTokenCoordinator.swift; sourceTree = "<group>"; };
DAB19D442B2C72A100DD08C4 /* AddCustomTokenViewModel.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = AddCustomTokenViewModel.swift; sourceTree = "<group>"; };
DAB19D452B2C72A100DD08C4 /* AddCustomTokenView.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = AddCustomTokenView.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -3419,6 +3423,7 @@
5D3F77DB24BF56DC00E8695B /* TangemTests */ = {
isa = PBXGroup;
children = (
DAAF69492BFE1CFA00179697 /* UserWalletNameIndexationTests.swift */,
0A1992A52B5FBDF800344312 /* CurrencyTests.swift */,
5D3F77DC24BF56DC00E8695B /* TangemTests.swift */,
DC4BF174298A4B810052EC19 /* IncomingActionsTests.swift */,
Expand Down Expand Up @@ -5910,6 +5915,14 @@
path = UserWalletStorageAggreement;
sourceTree = "<group>";
};
DAAF69472BFE1C0400179697 /* UserWalletNameIndexationHelper */ = {
isa = PBXGroup;
children = (
DAAF69462BFE1C0400179697 /* UserWalletNameIndexationHelper.swift */,
);
path = UserWalletNameIndexationHelper;
sourceTree = "<group>";
};
DAB5E331297E8CBF00A4E9CF /* Rounding */ = {
isa = PBXGroup;
children = (
Expand Down Expand Up @@ -6146,6 +6159,7 @@
DC0A57FF28243D050031BECC /* ExchangeService */,
DC0A57E62822AB8C0031BECC /* PersistentStorage */,
DC0A57E52822AA240031BECC /* UserWalletRepository */,
DAAF69472BFE1C0400179697 /* UserWalletNameIndexationHelper */,
DC0A57E02822A5A10031BECC /* TangemSdk */,
DC0A5848282C07750031BECC /* WalletConnect */,
DC0A57F52822BDFB0031BECC /* CardImageLoader */,
Expand Down Expand Up @@ -9291,6 +9305,7 @@
DC3B49F62893E51B00C207EA /* UserWalletConfig.swift in Sources */,
DADBECE32BBE910C007E7885 /* CustomFeeServiceInputOutput.swift in Sources */,
B6C4D7322B1A71480084AF1E /* BottomScrollableSheet+Environment.swift in Sources */,
DAAF69482BFE1C0400179697 /* UserWalletNameIndexationHelper.swift in Sources */,
B03156AA2A8636CB001D9646 /* TokenItemViewState+WalletModelState.swift in Sources */,
5D242A5F273C1F24008F79B5 /* Publisher+.swift in Sources */,
B0A0E7972BF48CAF0059FD5C /* VisaAppUtilities.swift in Sources */,
Expand Down Expand Up @@ -9827,6 +9842,7 @@
B03AC9932BD8DB26000CBC50 /* APIListTests.swift in Sources */,
DC4BF175298A4B810052EC19 /* IncomingActionsTests.swift in Sources */,
0A1992A62B5FBDF800344312 /* CurrencyTests.swift in Sources */,
DAAF694A2BFE1CFA00179697 /* UserWalletNameIndexationTests.swift in Sources */,
EF6221B3292B5FCD00D1D4A0 /* DecimalNumberFormatterTests.swift in Sources */,
DC7AC96A292BE2C900580668 /* TangemTests.swift in Sources */,
);
Expand Down
104 changes: 104 additions & 0 deletions TangemTests/UserWalletNameIndexationTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
//
// UserWalletNameIndexationTests.swift
// TangemTests
//
// Created by Andrey Chukavin on 17.05.2024.
// Copyright © 2024 Tangem AG. All rights reserved.
//

import XCTest
@testable import Tangem

class UserWalletNameIndexationTests: XCTestCase {
private var existingNameTestCases: [(String, String)] {
[
("Wallet 2", /* -> */ "Wallet 2"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

если уже есть индекс его пропускаем, на дублирование не обращаем внимания? Может обсуждали где-то? в описании задачи не нашел

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Показывал Лехе результат миграции, он одобрил

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wallet, Wallet, Wallet2 превратятся в Wallet2, Wallet3, Wallet2 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wallet -> Wallet (потому что такого нет)
Wallet -> Wallet 3 (потому что 2 уже есть)
Wallet2 -> Wallet 2 (потому что пользователь задал имя)

В первом куске тестов есть подобное

("Wallet", /* -> */ "Wallet"),
("Wallet 2", /* -> */ "Wallet 2"),
("Wallet", /* -> */ "Wallet 3"),
("Wallet", /* -> */ "Wallet 4"),
("Note", /* -> */ "Note"),
("Note", /* -> */ "Note 2"),
("Note", /* -> */ "Note 3"),
("Twin 1", /* -> */ "Twin 1"),
("Twin", /* -> */ "Twin 2"),
("Twin 3", /* -> */ "Twin 3"),
("Twin", /* -> */ "Twin 4"),
("Start2Coin 1", /* -> */ "Start2Coin 1"),
("Start2Coin 1", /* -> */ "Start2Coin 1"),
("Start2Coin 1", /* -> */ "Start2Coin 1"),
("Tangem Card", /* -> */ "Tangem Card"),
("Wallet 2.0", /* -> */ "Wallet 2.0"),
("Wallet 2.0", /* -> */ "Wallet 2.0 2"),
("Wallet 2.0", /* -> */ "Wallet 2.0 3"),
]
}

private var newNameTestCases: [(String, String)] {
[
("Wallet", "Wallet 5"),
("Note", "Note 4"),
("Twin", "Twin 5"),
("Start2Coin", "Start2Coin 2"),
("Wallet 2.0", "Wallet 2.0 4"),
("Tangem Card", "Tangem Card 2"),
]
}

func testUserWalletNameIndexation() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Давай еще пожалуйста тесты упростим, чтобы было в стиле

  1. тесты индексации нового кошелька
  • дано имя и пользовательский набор кошельков
  • ожидаемый результат такой-то
  • запустили, сравнили ассертом
  1. тесты миграции
  • дано: пользовательский массив
  • ожидаемый результат
  • сравнение

Все на хардкодах, без рандомных числе и тп

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тесты фактически на хардкодах, там рандом с сидом

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

тесты чем тупее написаны, тем лучше)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В этих тестах ничего особо умного нет. Это тоже самое что 10 захардкоженных тесткейсов. Если так сильно режет глаза могу на 10 массивов заменить

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Дело не в режет. Беглого взгляда на тесты недостаточно, чтобы понять что тестируется и какой ожидаемый результат. Не хочется тратить время на ревью еще и тестов

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Отрефакторил

let numberOfShuffleTests = 10

for testNumber in 1 ... numberOfShuffleTests {
var generator = SeededNumberGenerator(seed: testNumber, length: existingNameTestCases.count)
XCTAssertNotNil(generator)

let existingNames = existingNameTestCases.map(\.0).shuffled(using: &generator!)
let expectedNamesAfterMigration = existingNameTestCases.map(\.1).sorted()

let nameMigrationHelper = UserWalletNameIndexationHelper(mode: .migration, names: existingNames)

let migratedNames = existingNames
.map { name in
nameMigrationHelper.suggestedName(name)
}
.sorted()
XCTAssertEqual(migratedNames, expectedNamesAfterMigration)

for newNameTestCase in newNameTestCases {
XCTAssertEqual(nameMigrationHelper.suggestedName(newNameTestCase.0), newNameTestCase.1)
}

let newNameHelper = UserWalletNameIndexationHelper(mode: .newName, names: migratedNames)
for newNameTestCase in newNameTestCases {
XCTAssertEqual(newNameHelper.suggestedName(newNameTestCase.0), newNameTestCase.1)
}
}
}
}

private class SeededNumberGenerator: RandomNumberGenerator {
private let values: [UInt64]
private var index: Int = 0

init?(seed: Int, length: Int) {
guard length >= 1 else { return nil }

srand48(seed)

values = (1 ... length)
.map { _ in
let randomValue = drand48()
return UInt64(randomValue * Double(UInt64.max - 1))
}
}

func next() -> UInt64 {
let value = values[index]
if index < values.count - 1 {
index += 1
} else {
index = 0
}
return value
}
}
Loading