Skip to content
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

Merged
merged 40 commits into from
Oct 5, 2022

Conversation

perunt
Copy link
Contributor

@perunt perunt commented Aug 16, 2022

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 a react-native-keychain makes a deadlock when opening auth modal. It happens because react-native-keychain doesn't support a kind of concurrent mode but just makes a deadlock when we try to call getInternetCredentials 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 into react-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 all
In 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

  • Assigned individual reviewers?
  • Added labels? (team1/team2, critical path, release, dev QA)
  • Did you test both iOS and Android?
  • If your changes are visual, did you check both the light and dark themes?
  • Added e2e tests? If not, please specify why
  • If you added new critical path files, did you update the CODEOWNERS file?
  • If no dev QA label, did you add the PR to the QA Queue?

@perunt perunt added needs dev review Includes code review AND testing out the branch android android public beta dev QA PR that can be safely QA'd by devs for merge (feature-specific limited impact). QA during regression labels Aug 16, 2022
@linear
Copy link

linear bot commented Aug 16, 2022

RNBW-4267 Getting a crash when trying to speed up a txn

7b76fe08-76b3-4c1c-8c7d-2e488aafa984

Copy link
Member

@brunobar79 brunobar79 left a 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.

@terrysahaidak terrysahaidak removed the dev QA PR that can be safely QA'd by devs for merge (feature-specific limited impact). QA during regression label Aug 16, 2022
@terrysahaidak terrysahaidak removed the needs dev review Includes code review AND testing out the branch label Aug 22, 2022
@BrodyHughes
Copy link
Member

screen-20220822-095241.mp4

Hey @perunt ! Still getting this crash. Here is a video. I logged the error using adb logcat and I'll DM it to you.

Copy link
Member

@BrodyHughes BrodyHughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^^

@perunt perunt marked this pull request as draft August 23, 2022 08:23
@perunt perunt marked this pull request as ready for review August 23, 2022 18:10
Copy link
Member

@BrodyHughes BrodyHughes left a 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.

Copy link
Member

@brunobar79 brunobar79 left a 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

@terrysahaidak
Copy link
Contributor

/rebase

@github-actions github-actions bot force-pushed the @perunt/rnbw-4267-speed-up-txn branch from 414c7f4 to 1324b07 Compare September 28, 2022 20:12
@perunt perunt merged commit fa08ad3 into develop Oct 5, 2022
@perunt perunt deleted the @perunt/rnbw-4267-speed-up-txn branch October 5, 2022 11:28
skylarbarrera added a commit that referenced this pull request Oct 17, 2022
nickbytes pushed a commit that referenced this pull request Oct 18, 2022
nickbytes pushed a commit that referenced this pull request Oct 18, 2022
estrattonbailey pushed a commit that referenced this pull request Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android android public beta team1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants