-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix: android secure storage #803
Conversation
@camerow can you, please, try it on your device? |
apps/mobile/src/store/key-store.ts
Outdated
const { mnemonic, passphrase } = await mnemonicStore(fingerprint).getMnemonic(); | ||
|
||
if (!mnemonic) throw new Error('No mnemonic found for fingerprint ' + fingerprint); | ||
async deriveNextAccountKeychainsFrom( |
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.
How about having two functions.
deriveNextAccountKeychainsFromFingerprint
deriveNextAccountKeychains
- acceptsmnemonic
+passphrase
and callsderiveNextAccountKeychainsFromFingerprint
?
I think that will make it cleaner and easier to follow
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.
+1
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.
Yea but the problem is that i need to have mnemonic in deriveNextAccountKeychainsFromFingerprint
function, such that i'd be able to get a rootKeychain there. And i'm trying not to call mnemonicStore(props.fingerprint).getMnemonic()
again so that we don't display a biometrics check multiple times.
So I need to have a function that would have mnemonic, password and fingerprint received as props. But, I don't want to call that function with all of the props as i can derive fingerprint from mnemonic (using biometrics) and vice versa (without biometrics). This way, i can have an overloaded function that can either receive fingerprint or mnemonic, and with that it would be able to derive next account keychains.
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.
Nice work Edgar! I added some suggestions on cleaning the code up a bit.
It would be also good to:
- add a maestro test or two to confirm it works
- create Linear tasks for the outstanding issues you mentioned
- link this PR to the secure storage task
@edgarkhanzadian I'm getting a blank dead screen when creating a new wallet from zero-state freshly installed application after enabling secure mode.
After trying a few times the screen was no longer blank but the application crashes when I press the "I've backed it up" button. |
yeap that's the blur view issue happening, it's happening on dev too so will fix as part of a separate PR
If that happens on dark mode, then it should be lottie related |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #803 +/- ##
=======================================
Coverage 29.07% 29.07%
=======================================
Files 204 204
Lines 8132 8132
Branches 560 560
=======================================
Hits 2364 2364
Misses 5768 5768
|
@edgarkhanzadian so this isn't quite testable in the current state of the app? |
@camerow alrighty, with those 2 fixes now in, this PR should be ready for testing |
fad950a
to
0818a42
Compare
@edgarkhanzadian tested and this is working for me now! Nice one. 🚀 |
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.
Great work @edgarkhanzadian
c862bdc
to
b3beee3
Compare
b3beee3
to
d8c1f7e
Compare
Secure mode now works also for PIN, password, or pattern.
Things that need to get merged into this PR: