Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/BIP44 #719
Feature/BIP44 #719
Changes from 19 commits
8d5411b
08ddc4f
43f339d
d31fb83
7cb0fb6
aa14c74
2f537c2
29cda62
48716c8
7d38122
3250d72
0842f35
b50550d
4369dcc
50f0986
73e2c1b
9e28c97
5088e51
7c9ca5f
10a3270
4783cb2
825f11d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 callhasTransactions
function we check that the address is valid we do not provide safety for others if they want to implement a version ofTransactionChecker
themselves.Any thoughts are welcome!
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.
Looks reasonable :)
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.
Could you add at least a field like
hash
to theTransaction
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I had no time yesterday.