IOS-7738 Add WalletManager #818

amuraveinik · 2024-08-30T12:05:20Z

TODO: Учесть комменты отсюда ✅

Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…ain" This reverts commit 43be0dc, reversing changes made to bb770e1.

…twork_provider

…ansaction_builder

…ture/IOS-7738_filecoin_wallet_manager

amuraveinik · 2024-08-31T16:01:29Z

BlockchainSdk/Blockchains/Filecoin/FilecoinExternalLinkProvider.swift

-        /// However, in Filecoin, message IDs (which are used to identify transactions) are not derived from transaction hashes.
-        /// Therefore, constructing a URL in the format `"\(baseExplorerUrl)/message/\(hash)"` is not applicable.
-        nil
+        URL(string: "\(baseExplorerUrl)/message/\(hash)")


По этому пункту поступила обновленная информация от андроида

amuraveinik · 2024-08-31T16:01:56Z

BlockchainSdk/Blockchains/Filecoin/FilecoinFeeParameters.swift

    let gasLimit: Int64
+    let gasFeeCap: BigUInt


Просто переименование, чтобы матчилось с тем что приходит с апишки

BlockchainSdk/Blockchains/Filecoin/FilecoinWalletManager.swift

BlockchainSdk/Extensions/JSONDecoder+.swift

Balashov152 · 2024-09-03T10:56:39Z

BlockchainSdk/Blockchains/Filecoin/FilecoinTransactionBuilder.swift

+        // // https://github.com/tangem/wallet-core/blob/996bd5ab37f27e7f6e240a4ec9d0788dfb124e89/src/PublicKey.h#L35
+        let v = BigUInt(unmarshal.v) - 27
+        let encodedV = v == .zero ? Data([UInt8.zero]) : v.serialize()
+        let signature = unmarshal.r + unmarshal.s + encodedV


здесь скорее всего просто unmarshal должно хватить, так как EIP-155 как будто не валиден для Filecoin

Нужно так как сейчас, иначе не проходит валидация подписи

Поправил ниже по комментам Андрея Ф

m3g0byt3

По вот этим комментам не нашел в этом пуле ничего

#816 (comment)
#816 (comment)

Или не туда смотрю?

BlockchainSdk/Blockchains/Filecoin/FilecoinTransactionBuilder.swift

BlockchainSdk/Blockchains/Filecoin/FilecoinWalletManager.swift

m3g0byt3 · 2024-09-03T12:00:50Z

BlockchainSdk/Blockchains/Filecoin/FilecoinWalletManager.swift

+        let message = FilecoinMessage(
+            from: wallet.address,
+            to: destination,
+            value: bigUIntValue.description,


А у biguint нет какого-нибудь метода для перегона в строку?
description все-таки для другого и его формат недедтерминирован

По моему эта проперти специально для этого и предназначена

extension BigUInt: CustomStringConvertible { /// Return the decimal representation of this integer. public var description: String { return String(self, radix: 10) } }

вообще сериализация использовалась, а тут надо именно строку в десятичном формате?

В десятичном виде нужна

Явно вызывать String(self, radix: 10) выглядит корректней, на этот конструктор наверняка написаны тесты и т.д.
А description нужен по сути только для того, чтобы в логах распечатать и его реализация не гарантируется
Ну и оффдока:

Accessing a type’s description property directly or using CustomStringConvertible as a generic constraint is discouraged.

Впрочем не крит

BlockchainSdk/Blockchains/Filecoin/FilecoinWalletManager.swift

BlockchainSdk/Blockchains/Filecoin/Network/DTO/FilecoinResponse.swift

amuraveinik · 2024-09-03T12:24:49Z

По вот этим комментам не нашел в этом пуле ничего

#816 (comment) #816 (comment)

Или не туда смотрю?

Не в полной мере актуальны комменты здесь

#816 (comment)

А какой тип у этого поля в апишке? Если float - то получается потеря точности. Если string - то корректнее маппить в decimal в маппере/конвертере

Возвращается тип - строка. В FilecoinAccountInfo (это там где balance: Decimal) значение из апишики сразу не парсится, оно как раз маппится в нетворк сервисе. Отдельно заводить маппер на однострочную операцию показалось оверкиллом

#816 (comment)

А тут хватает uint64? В чем номинируется цена газа, я у них на сайте нашел упоминание про attoFIL, не в них случайно? Если да, то 1 attoFIL всего в 18 раз меньше uint64.max

Самого этого запроса больше нет, поменял на другой. Из апишки возвращается строка, паршу ее в Decimal. attoFIL да

amuraveinik · 2024-09-03T13:59:35Z

@m3g0byt3 Про пендинг. Почему то не получилось к тому сообщение прилинковать ответ.

Вообще в доке есть метод MpoolCheckPendingMessages, и он идеально бы подошел
https://docs.filecoin.io/reference/json-rpc/mpool#mpoolcheckpendingmessages

Но апишка на запрос всегда возвращает ошибку the method Filecoin.MpoolCheckPendingMessages does not exist/is not available
На несуществующий метод всегда возвращается другая ошибка, поэтому это выглядит просто как неработающий запрос.

Предлагаю пока что оставить как есть до момента пока явно не будет сказано для файлкоина сделать возможность параллельной отправки

…llet_manager

tureck1y

Нужно QA попросить сделать смоук вичейна

Balashov152 · 2024-09-03T14:49:37Z

BlockchainSdk/Blockchains/Filecoin/FilecoinWalletManager.swift

+                FilecoinMessage(
+                    from: address,
+                    to: destination,
+                    value: bigUIntValue.description,


точно description? не надо там serialized() ?

Выше про это
#818 (comment)

m3g0byt3 · 2024-09-03T14:45:53Z

BlockchainSdk/Blockchains/VeChain/VeChainTransactionBuilder.swift

+        /// VeChain is a fork of `Geth Classic`, so it expects the secp256k1's `recid` to have values in the 0...3 range.
+        /// Therefore we have to convert value of the standard secp256k1's `recid` to match this expectation.


nit: теперь этот коммент и объяснения в нем скорее к самой утилите SignatureUtils относится, офк без упоминания там VeChain
А просто что есть некоторые такие блокчейны, для которых нужно делать SignatureUtils.unmarshalledSignature

m3g0byt3 · 2024-09-03T14:48:26Z

BlockchainSdk/Blockchains/Filecoin/FilecoinWalletManager.swift

+                walletManager.wallet.addPendingTransaction(record)
+                return TransactionSendResult(hash: txId)
+            }
+            .eraseSendError()


Кажется tx_id нужен в SendError, а тут он теряется

В остальных блокчейнах же не случайно делалась эта доработка, чтобы SendError содержал хеш транзы

Я приложил в прошлом комменте оба метода в экстеншене, другие сети сначала маппят в SendError с сохранением tx_id через mapSendError
А уже затем эрайзят через eraseSendError

@skibinalexander @tureck1y должны быть в контексте, зачем в SendError кладется хэш

m3g0byt3 · 2024-09-03T14:54:46Z

BlockchainSdk/Blockchains/Filecoin/FilecoinWalletManager.swift

+        let message = FilecoinMessage(
+            from: wallet.address,
+            to: destination,
+            value: bigUIntValue.description,


Явно вызывать String(self, radix: 10) выглядит корректней, на этот конструктор наверняка написаны тесты и т.д.
А description нужен по сути только для того, чтобы в логах распечатать и его реализация не гарантируется
Ну и оффдока:

Accessing a type’s description property directly or using CustomStringConvertible as a generic constraint is discouraged.

Впрочем не крит

amuraveinik · 2024-09-03T16:33:09Z

BlockchainSdk/Blockchains/Filecoin/FilecoinWalletManager.swift

+            .flatMap { walletManager, message in
+                walletManager.networkService
+                    .submitTransaction(signedMessage: message)
+                    .mapSendError(tx: try? JSONEncoder().encode(message).utf8String)


Тут именно так, потому что в апишку отправляем json объект, а не любого иного рода строку

tureck1y · 2024-09-03T17:16:27Z

BlockchainSdk/Common/Utils/SignatureUtils.swift

@@ -9,6 +9,24 @@
 import TangemSdk

 enum SignatureUtils {
+    /// Unmarshals a signature using the provided original signature, public key, and hash.


Это все будет полностью переделано в будущем и перенесено в TangemSdk

amuraveinik and others added 14 commits August 22, 2024 16:44

IOS-7718 Add Filecoin blockchain

7516e93

IOS-7718 Revert Package.resolved

bb770e1

Bump rexml from 3.2.8 to 3.3.6 (#811)

b81a6ce

Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Merge branch 'develop' into feature/IOS-7718_filecoin_blockchain

43be0dc

Revert "Merge branch 'develop' into feature/IOS-7718_filecoin_blockch…

bf272b3

…ain" This reverts commit 43be0dc, reversing changes made to bb770e1.

IOS-7718 Fix comment

c43e5b0

IOS-7718 Add TODO for WalletAssembly

f897f62

IOS-7718 Fix tests

503be34

IOS-7718 Add AddressTests

69202d6

IOS-7718 Add NetworkProvider

66bba5b

Merge branch 'blockchains/filecoin' into feature/IOS-7718_filecoin_ne…

490d283

…twork_provider

IOS-7718 Add TransactionBuilder

bc59262

IOS-7718 Fix address in tests

1462f26

IOS-7738 Add WalletManager

4e7918d

amuraveinik requested review from tureck1y, dbaturin and a team as code owners August 30, 2024 12:05

amuraveinik requested review from m3g0byt3 and Andoran90 and removed request for a team August 30, 2024 12:05

amuraveinik marked this pull request as draft August 30, 2024 12:05

amuraveinik added 5 commits August 30, 2024 14:18

Merge branch 'blockchains/filecoin' into feature/IOS-7718_filecoin_tr…

e8b538f

…ansaction_builder

IOS-7779 CR fixes

e0b4aa8

Merge branch 'feature/IOS-7718_filecoin_transaction_builder' into fea…

9d40b6e

…ture/IOS-7738_filecoin_wallet_manager

IOS-7738 Add manager

640b8c5

IOS-7738 Change naming and data types

38e53c7

amuraveinik commented Aug 31, 2024

View reviewed changes

amuraveinik added 3 commits September 2, 2024 19:29

IOS-7738 Fix transaction sending

c63f413

IOS-7738 Adjust tests

e047908

IOS-7738 Remove not used

9fbe38b

Base automatically changed from feature/IOS-7718_filecoin_transaction_builder to blockchains/filecoin September 3, 2024 10:29

Balashov152 reviewed Sep 3, 2024

View reviewed changes

m3g0byt3 reviewed Sep 3, 2024

View reviewed changes

IOS-7738 CR fixes

b1d2d00

Merge branch 'blockchains/filecoin' into feature/IOS-7738_filecoin_wa…

167b733

…llet_manager

amuraveinik requested review from tureck1y, m3g0byt3 and Balashov152 September 3, 2024 14:20

tureck1y previously approved these changes Sep 3, 2024

View reviewed changes

Balashov152 previously approved these changes Sep 3, 2024

View reviewed changes

m3g0byt3 reviewed Sep 3, 2024

View reviewed changes

IOS-7438 CR Fixes

81e9368

amuraveinik dismissed stale reviews from Balashov152 and tureck1y via 81e9368 September 3, 2024 16:28

amuraveinik requested review from m3g0byt3, tureck1y and Balashov152 September 3, 2024 16:28

IOS-7438 Change tx

5ef0aab

amuraveinik commented Sep 3, 2024

View reviewed changes

tureck1y reviewed Sep 3, 2024

View reviewed changes

tureck1y approved these changes Sep 3, 2024

View reviewed changes

m3g0byt3 approved these changes Sep 4, 2024

View reviewed changes

Balashov152 approved these changes Sep 4, 2024

View reviewed changes

amuraveinik enabled auto-merge (squash) September 4, 2024 10:53

Andoran90 approved these changes Sep 4, 2024

View reviewed changes

amuraveinik merged commit 0cb170c into blockchains/filecoin Sep 4, 2024
1 check passed

amuraveinik deleted the feature/IOS-7738_filecoin_wallet_manager branch September 4, 2024 10:56

amuraveinik mentioned this pull request Sep 4, 2024

IOS-7438 Filecoin #824

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IOS-7738 Add WalletManager #818

IOS-7738 Add WalletManager #818

amuraveinik commented Aug 30, 2024 •

edited

Loading

amuraveinik Aug 31, 2024

amuraveinik Aug 31, 2024

Balashov152 Sep 3, 2024

amuraveinik Sep 3, 2024

amuraveinik Sep 3, 2024

m3g0byt3 left a comment

m3g0byt3 Sep 3, 2024

amuraveinik Sep 3, 2024

tureck1y Sep 3, 2024

amuraveinik Sep 3, 2024

m3g0byt3 Sep 3, 2024

amuraveinik commented Sep 3, 2024 •

edited

Loading

amuraveinik commented Sep 3, 2024 •

edited

Loading

tureck1y left a comment

Balashov152 Sep 3, 2024

amuraveinik Sep 3, 2024

m3g0byt3 Sep 3, 2024

m3g0byt3 Sep 3, 2024

m3g0byt3 Sep 3, 2024

amuraveinik Sep 3, 2024

tureck1y Sep 3, 2024

		/// VeChain is a fork of `Geth Classic`, so it expects the secp256k1's `recid` to have values in the 0...3 range.
		/// Therefore we have to convert value of the standard secp256k1's `recid` to match this expectation.

IOS-7738 Add WalletManager #818

IOS-7738 Add WalletManager #818

Conversation

amuraveinik commented Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3g0byt3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amuraveinik commented Sep 3, 2024 • edited Loading

amuraveinik commented Sep 3, 2024 • edited Loading

tureck1y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amuraveinik commented Aug 30, 2024 •

edited

Loading

amuraveinik commented Sep 3, 2024 •

edited

Loading

amuraveinik commented Sep 3, 2024 •

edited

Loading