Feature/BIP44 #719
Feature/BIP44 #719
Conversation
…, change and addressIndex from a valid bip44 path. code cleanup
…previous accounts have transactions
albertopeam
force-pushed
the
feature/add-bip-44
branch
from
46adcea
to
48716c8
Compare
|
having some issues with swiftlint... I expect to fix them soon |
|
fixed swiftlint issues |
Sources/Web3Core/KeystoreManager/EtherscanTransactionChecker.swift
Outdated
Show resolved
Hide resolved
|
Just to mention: I'm actually waiting when @JeneaVranceanu issues would be applied to review it by myself. |
Sources/Web3Core/KeystoreManager/EtherscanTransactionChecker.swift
Outdated
Show resolved
Hide resolved
Sources/Web3Core/KeystoreManager/EtherscanTransactionChecker.swift
Outdated
Show resolved
Hide resolved
…URLSessionProxy from public to internal
|
@JeneaVranceanu please take a look at the last state of this one, it would be great to include it into 3.1.0 release. |
albertopeam
requested review from
yaroslavyaroslav and
JeneaVranceanu
and removed request for
yaroslavyaroslav
Sources/Web3Core/KeystoreManager/EtherscanTransactionChecker.swift
Outdated
Show resolved
Hide resolved
albertopeam
requested review from
yaroslavyaroslav and
JeneaVranceanu
and removed request for
JeneaVranceanu and
yaroslavyaroslav
@yaroslavyaroslav On it right now. |
|
|
||
| public protocol BIP44 { | ||
| /** | ||
| Derive an ``HDNode`` based on the provided path. The function will throw ``BIP44Error.warning`` if it was invoked with throwOnWarning equal to |
There was a problem hiding this comment.
Please, wrap throwOnWarning into backticks so that it stands out when reading this as compiled documentation.
There was a problem hiding this comment.
I'll highlight such issues as I go.
| public protocol BIP44 { | ||
| /** | ||
| Derive an ``HDNode`` based on the provided path. The function will throw ``BIP44Error.warning`` if it was invoked with throwOnWarning equal to | ||
| `true` and the root key doesn't have a previous child with at least one transaction. If it is invoked with throwOnError equal to `false` the child node will be |
There was a problem hiding this comment.
throwOnWarning wrap with backticks, please.
There was a problem hiding this comment.
On line 12 as well.
| - Throws: any error related to query the blockchain provider | ||
| - Returns: `true` if the address has at least one transaction, `false` otherwise | ||
| */ | ||
| func hasTransactions(address: String) async throws -> Bool |
There was a problem hiding this comment.
Let's change the type of address to EthereumAddress.
This way we reduce one potential error - an invalid ethereum address.
The fact that we implement TransactionChecker and before we call hasTransactions function we check that the address is valid we do not provide safety for others if they want to implement a version of TransactionChecker themselves.
Any thoughts are welcome!
There was a problem hiding this comment.
Looks reasonable :)
| struct Response: Codable { | ||
| let result: [Transaction] | ||
| } | ||
| struct Transaction: Codable {} |
There was a problem hiding this comment.
Could you add at least a field like hash to the Transaction struct?
Or maybe a swift doc to that struct explaining why it's empty to make sure we won't spend time in the future thinking too much about why it's empty. 🙂
I guess it is something like We are not interested in the contents of the transaction object but rather the availability of the transaction per see for the sake of counting the number of transactions for a specific account..
There was a problem hiding this comment.
Apologies, I had no time yesterday.
|
@albertopeam There is only one comment that really concerns me: #719 (comment) The rest is actually looking very good. |
|
Sweet, now this was the last one within 3.1.0 I'll release it tomorrow or at monday. So please, folks, don't merge anything into dev right away. @JeneaVranceanu @janndriessen |
Summary of Changes
Adds bip44 to HDNode
Test Data or Screenshots
By submitting this pull request, you are confirming the following:
developbranch.