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

fix: rebuild iOS structure to build in master #725

Merged
merged 9 commits into from Feb 19, 2021
Merged

fix: rebuild iOS structure to build in master #725

merged 9 commits into from Feb 19, 2021

Conversation

TarikGul
Copy link
Contributor

@TarikGul TarikGul commented Feb 16, 2021

Info

This PR has an aim to fix current bugs with the current master on Parity-Signer. The idea behind this PR is also to help move the project forward so that we can introduce this PR into the next release. This is mainly focusing on creating a stable repo for future development for iOS. Since the last PR in September, both iOS 14, along with xcode 12 was introduced. This caused a lot of issues for development ie, cocoapods, dependencies, and the QRcode not showing up.

Bugs Found

  1. On master when running iOS there is a bug that causes the QRcode from not showing up. This is due to an issue with React Native 0.62.x and iOS 14.x where due to an animation issue an image wont appear on a mounted component.
    • [THE FIX] Update React-Native to 0.63.4. Below I will mention the residual issues that this causes and the fixes made to accompany it.
    • [NOTE] This fix has residual effects such as Parity-Signer will not be compatible with iOS 9. Meaning I switched the compatibility of the Podfile up to iOS 10 -14.x. iOS 10 is supported down to iPhone 5.
  2. Along with the QRcode not showing up, iOS would have problems building as soon as React-Native was updated due to some of the Pods (I believe but no 100% sure that it is also because of xcode 12 but I will get into that further down the PR).
    • [THE FIX] Add React native scripts to the Podfile, specify the Flipper-Folly version to 2.3.0, and change the path for the React-Callinvoker
  3. Jest. Now the local jest tests would not locate the jest.config.js and would cause errors regarding a lack of mock exports for an image. At first I thought I was crazy because the pathing seemed all correct, until I noticed it wasn't actually ever reading the jestCommonConfig.js.
    • [THE FIX] I added the pathing inside of the package.json to correctly identify the jest.config and run the tests accordingly. Now all you have to do is yarn jest and you can unit test locally.
  4. The way e2e testing is done is it calls yarn build-e2e:ios which calls yarn xcbuild:debug. Now this was causing some catastrophic errors when building. This has to directly do with the xcode 12 update as well, as iOS 14.4 and React Native 0.63.4 and the arm64 for simulator architecture.
    • [THE FIX] EXCLUDE arm64 simulator architecture for both the xcode project and pod project. Further information on this bug can be seen here
  5. The last iOS bug found was once the build-e2e:ios is complete, the previous version of detox(<=16.xx.x) was not working well with xcode 12, and iOS 14.x. Detox as a library is reliant on a package called EarlGrey which they are slowly transitioning off of and which was causing our e2e-testing issues.
    • [THE FIX] Upgraded to Detox@18.6.0 which is working for us, and is less dependant on EarlGrey.
    • [NOTE] It is always important to note to have the react-native server running in the background when running the e2e-tests locally. Or else Detox will fail everything because it cannot read from the component hierarchy.

@blockchainunchained
Copy link

blockchainunchained commented Feb 17, 2021

This fix has residual effects such as Parity-Signer will not be compatible with iOS 9. Meaning I switched the compatibility of the Podfile up to iOS 10 -14.x. iOS 10 is supported down to iPhone 5.

This isn't a problem. Most people have moved on from iPhone 4 devices now, even as their backup / spare device. Chances are the majority of them no longer take a charge and the batteries aren't easy to replace. It is worth noting that iPhone 6 or below should be considered insecure and can be hacked trivially. The 6s is slightly better but only XR / XS or above is considered safe.

The e2e build process works fine now locally

Great work! Well done.

@TarikGul
Copy link
Contributor Author

@blockchainunchained Hey thanks for the info!

It is worth noting that iPhone 6 or below should be considered insecure and can be hacked trivially.

This is great to know and I'll make sure to do some more research myself to get a better understanding, and ask our security team as well. When looking through the older bug patches I did see a lot of issues with the older iPhones so, I will make sure to document them as I find them and as we move forward and closer to a release hopefully we will have some solid documentation.

Great work! Well done.

Thank you, it means a lot. I dropped everything to make sure Signer moves forward and productive steady progress is made. Keep an eye out for these PR's, and updates on here as I will be working mostly on Signer until it's in a good place to pass on to future maintainers.

@blockchainunchained
Copy link

I dropped everything to make sure Signer moves forward and productive steady progress is made

In which case you have almost certainly prevented users from losing funds unnecessarily, let's check everything twice and move quickly but cautiously. I have spare devices (including old iPhones) and I am more than happy to help test.

I've never worked with Rust but I have a working knowledge of React Native, unfortunately that means I can't be of much further assistance than testing right now. I'll try and work on that.

@blockchainunchained
Copy link

Don't Parity / Polkadot / Web3 have a security company on retainer? Is this project in scope? Can it be if not?

@TarikGul
Copy link
Contributor Author

Thanks for offering up your time to test and help. Currently the goal of this specific PR is to update the Repo so that it is up to date specifically for iOS so that anyone can pull it down and have a working repo to build and test on. So once this is merged then the focus will be towards testing on devices and getting this PR operational with up to date metadata and so on so transactions can be decoded, and viewed before signing.

Don't Parity / Polkadot / Web3 have a security company on retainer? Is this project in scope? Can it be if not?

To be honest I have no idea.

@dvdplm
Copy link
Contributor

dvdplm commented Feb 19, 2021

Don't Parity / Polkadot / Web3 have a security company on retainer? Is this project in scope? Can it be if not?

We do and this is a great suggestion. The scope does not cover Signer afaik so it's not going to happen first thing on Monday, but I'll see what we can do.

Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

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

🚀

@dvdplm dvdplm merged commit 0052891 into master Feb 19, 2021
@dvdplm dvdplm deleted the fix-ios branch February 19, 2021 10:26
@TriplEight
Copy link
Contributor

What about iphone SE 1 gen? I know it's widely used as a backup device and is still supported.

@TriplEight
Copy link
Contributor

@TarikGul I've sent you an invite to Bitrise, which I was configuring as a state of the art CI for Signer, we could discuss what can be done better there.

@TarikGul
Copy link
Contributor Author

@TriplEight Awesome just accepted it. Also the Iphone SE 1 gen will definitely work for it.

@TriplEight
Copy link
Contributor

np.
I mean I know it's going to work, but for how long? xD

@TarikGul
Copy link
Contributor Author

Oh my bad :) It Shouldn't be an issue for a very very long time, the SE is compatible with all the iOS version's above 10 , so I don't see react-native dropping support for it for many years to come. Thanks again for the Bitrise access.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants