-
Notifications
You must be signed in to change notification settings - Fork 577
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
Remove a thread blocking behavior in react-native-keychain
#4025
Conversation
RNBW-4267 Getting a crash when trying to speed up a txn
|
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.
LGTM!
Not sure why e2e are failing tho.
screen-20220822-095241.mp4Hey @perunt ! Still getting this crash. Here is a video. I logged the error using |
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.
^^^
…@perunt/rnbw-4267-speed-up-txn
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.
No crashes happening.
Sped up 10 queued txns on one wallet then 6 queued txns on another on iPhone & Android.
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.
Changing to request changes since the keychain patch was introduced later after my initial review and had some comments about it
/rebase |
414c7f4
to
1324b07
Compare
…@perunt/rnbw-4267-speed-up-txn
…ainbow-me/rainbow into @perunt/rnbw-4267-speed-up-txn
Fixes RNBW-4267
Figma link (if any):
What changed (plus any additional context for devs)
AFAIK it happens when animation is not finished, and we call the
getInternetCredentials
method from the keychain(we use it for speed up transactions/canceling). It looks like areact-native-keychain
makes a deadlock when opening auth modal. It happens becausereact-native-keychain
doesn't support a kind ofconcurrent mode
but just makes a deadlock when we try to callgetInternetCredentials
at the same time as animation is performing.We decided to create a fork with these new changes, which will be fair for SDK >= 24. In the
index
file intoreact-native-keychain
we just decide which keychain version we wanna use - the forked one or the real keychain. So we don't need to change the js code at allIn the future, we'll be able just to drop the forked version, or we can update the fork with required changes from its "main."
forked lib: https://github.com/rainbow-me/react-native-keychain-new
PR, which I use for this fork: oblador/react-native-keychain#547
Here are changes in the fork: https://github.com/oblador/react-native-keychain/compare/master...rainbow-me:react-native-keychain-new:master?expand=1
Screen recordings / screenshots
What to test
Test everything related to the keychain, creating a new wallet, creating a backup, restoring a backup, accessing the seed phrase.
It is important to note that you need to test on two versions of Android, up to version 7 (5-6) and from version 7 (7 - up to date)
The fix should work fully for versions 7 and above
For versions 5-6, the fix will work partially, and the error can be reproduced, but much less often
Final checklist
team1/team2
,critical path
,release
,dev QA
)dev QA
label, did you add the PR to the QA Queue?