-
Notifications
You must be signed in to change notification settings - Fork 9
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-7779 Add TransactionBuilder #817
IOS-7779 Add TransactionBuilder #817
Conversation
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
|
||
func buildForSign(transaction: Transaction, nonce: UInt64) throws -> Data { | ||
guard let feeParameters = transaction.fee.parameters as? FilecoinFeeParameters else { | ||
throw WalletError.failedToBuildTx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Прикольно еще было бы поменять FilecoinTransactionBuilderError.filecoinFeeParametersNotFound
что бы если что проще искать было, в чем проблема, а то WalletError.failedToBuildTx
много где используется
throw WalletError.failedToBuildTx | ||
} | ||
|
||
return try JSONDecoder().decode(FilecoinSignedTransactionBody.self, from: jsonData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А там в API прям json будет уходить? обычно просто длинная hex строка
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feeParameters: FilecoinFeeParameters | ||
) throws -> FilecoinSigningInput { | ||
guard let value = transaction.amount.bigUIntValue else { | ||
throw WalletError.failedToBuildTx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
и тут тоже, ну во всех местах лучше бы FilecoinTransactionBuilderError
юзать)
The base branch was changed.
struct FilecoinFeeParameters: FeeParameters { | ||
let gasUnitPrice: BigUInt | ||
let gasLimit: Int64 | ||
let gasPremium: BigUInt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
По поводу типов данных - я провалидирую корректность в следующем PRе (про WalletManager)
@tureck1y @skibinalexander Влить бы задачку. |
input.gasLimit = feeParameters.gasLimit | ||
input.gasPremium = feeParameters.gasPremium.serialize() | ||
|
||
input.publicKey = try Secp256k1Key(with: wallet.publicKey.blockchainKey).decompress() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Лучше декомпресснуть один раз и положить в параметры класса
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Пофиксил в следующем PR
#818 (comment)
BlockchainSdk/Blockchains/Filecoin/Network/DTO/FilecoinSignedTransactionBody.swift
Show resolved
Hide resolved
import XCTest | ||
@testable import BlockchainSdk | ||
|
||
final class FilecoinTransactionBuilderTests: XCTestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не увидел теста на размер транзакции, хоть и секп все равно его надо делать
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Пофиксил в следующем PR
#818 (comment)
@siblockchaina Поставь повторно пожалуйста аппрув, слетел после допушиваний |
IOS-7779