-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implement Face ID / Touch ID + Refactors + The Great Divorce (cleanup unused code) #12
Conversation
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.
Self-review complete
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.
Because I've burnt with milk I'm pointing this cow out.
The LAAuthentication API plainly sucks (thanks apple!). You should mock it and test all the possible branches of the possible code with unit tests
Out of scope for a Code Review:
test live on Simulator and also try the same workflow on an iPhone that doesn't have a pin set up.
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.
utACK.
Resolved conversation with @tw0po1nt
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.
In as much as can be tested in the presence of the ZcashLightClientKit error, no issues apart from the below encountered. In an emulator I'm unable to test Face ID unlock specifically, though in lieu of this, the iPhone passcode unlock works as expected.
Not sure if new behaviour or existing:
- In wallets with 0 balance, entering a ZEC amount on the 'Send Money' screen expectedly presents a 'TOP UP WALLET' button instead. However, nothing happens upon tapping it.
- With no value entered, 'Scan a payment code' on this same screen is also non-functional.
Also ran into opportunities for the seed restore flow previously noted which I'll open an issue for.
Interesting, ok. I knew the scan a payment code wasn't working, as that's ZIP-321 and still todo. The top up wallet, as I recall, used to work. So I'll look into that prior to merging this. |
This code review checklist is intended to serve as a starting point for the author and reviewer, although it may not be appropriate for all types of changes (e.g. fixing a spelling typo in documentation). For more in-depth discussion of how we think about code review, please see Code Review Guidelines.
Author
Reviewer