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

[Wallet] Preliminary iOS support #1098

Merged
merged 35 commits into from
Sep 25, 2019
Merged

[Wallet] Preliminary iOS support #1098

merged 35 commits into from
Sep 25, 2019

Conversation

jeanregisser
Copy link
Contributor

@jeanregisser jeanregisser commented Sep 25, 2019

Description

This is the preliminary support for iOS 🍏📱🎉

Special thanks to @cmcewen and @tkporter who made geth work on iOS and paved the way for this!

Though the UI is "usable" it's relatively broken and needs fixing. Especially around keyboard handling and taking the notch/status bar into account.

Remaining work is tracked in https://docs.google.com/document/d/1n84U4Mgvut5V7VHlvKB2LfJSQ8RZkrTbUir04_PP0Zw/edit#

I will open individual issues for them.

To run on iOS:
cd packages/mobile
bundle install
cd ios; bundle exec pod install
react-native run-ios or manually open celo.xcworkspace and build and run.

Note: reloading RN breaks geth. You'll have to kill the app and relaunch for now. See valora-inc/react-native-geth@0713313

Tested

Ran the app on iOS and Android. Tested invite flow, sms verification, send money.

IMG_2689

Other changes

Related issues

Backwards compatibility

Yes.

tkporter and others added 30 commits August 15, 2019 16:14
See lugg/react-native-config#349
We still need to clean rebuild when changing values in .env though
Will address typo in another PR
react-native-secure-keystore supports it ouf of the box 🎉
Default platform is now Android for now.
- It now uses the same native module name for both iOS and Android.
- It already has our custom patch.
post_install do |installer|
installer.pods_project.targets.each do |target|
target.build_configurations.each do |config|
config.build_settings['ENABLE_BITCODE'] = 'NO'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Geth framework doesn't support bitcode. Not really a deal breaker, I tend to disable bitcode usually as it complicates crash reporting a little bit (you have to download the symbols after uploading the app to App Store Connect to be able to symbolicate stacktraces, It can (should) be automated anyway but takes longer in the CI pipeline).

@codecov
Copy link

codecov bot commented Sep 25, 2019

Codecov Report

Merging #1098 into master will increase coverage by <.01%.
The diff coverage is 35.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1098      +/-   ##
==========================================
+ Coverage   66.45%   66.45%   +<.01%     
==========================================
  Files         256      256              
  Lines        7349     7367      +18     
  Branches      488      491       +3     
==========================================
+ Hits         4884     4896      +12     
- Misses       2375     2379       +4     
- Partials       90       92       +2
Flag Coverage Δ
#mobile 66.45% <35.89%> (ø) ⬆️
Impacted Files Coverage Δ
packages/mobile/src/utils/time.ts 61.9% <0%> (-0.89%) ⬇️
packages/mobile/src/geth/geth.ts 24.07% <13.33%> (+0.35%) ⬆️
...ifications/EscrowedPaymentReminderNotification.tsx 74.28% <33.33%> (-2.19%) ⬇️
packages/mobile/src/verify/Input.tsx 67.3% <50%> (-1.32%) ⬇️
packages/mobile/src/web3/contracts.ts 68.96% <50%> (-8.31%) ⬇️
packages/mobile/src/components/DevSkipButton.tsx 79.16% <50%> (-3.45%) ⬇️
packages/mobile/src/invite/EnterInviteCode.tsx 81.33% <50%> (-1.1%) ⬇️
packages/mobile/src/geth/saga.ts 30.95% <66.66%> (+0.83%) ⬆️
packages/mobile/src/invite/saga.ts 85.61% <66.66%> (-0.6%) ⬇️
packages/mobile/src/pincode/PincodeUtils.ios.ts 0% <0%> (-75%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2929446...40c5c66. Read the comment docs.

Copy link
Contributor

@jmrossy jmrossy left a comment

Choose a reason for hiding this comment

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

Mostly looks great! Just a few small comments

package.json Show resolved Hide resolved
@@ -0,0 +1,8 @@
//
// Dummy.swift
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. Removing it.

@@ -0,0 +1,7 @@
export const setPin = async (pin: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I removed the PincodeViaAndroidKeystore file in my pincode PR. I don't think you need this file, it should be using PincodeUtils.ios.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed 👍

@@ -30,16 +30,21 @@ export default class ReactNativeLogger {
}
}

// TODO: remove this method from here and use our alert banner
Copy link
Contributor

Choose a reason for hiding this comment

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

No, there are a few cases when the design calls for a toast message instead of the blue banner. If iOS doesn't have an equivalent, maybe we'll need to make a generic toast component (or change the designs, which I'm personally open to)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iOS doesn't have a native equivalent. I'll update the comment and will check with Corey.

Copy link
Contributor

@jmrossy jmrossy left a comment

Choose a reason for hiding this comment

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

🚢 ☎️ 💯

package.json Show resolved Hide resolved
@jeanregisser jeanregisser merged commit 207fab3 into master Sep 25, 2019
@jeanregisser jeanregisser deleted the cmcewen/new-ios branch September 25, 2019 15:48
aaronmgdr added a commit that referenced this pull request Sep 25, 2019
* master: (61 commits)
  Remove locales as website is now just in English (#1050)
  Add MetadataURL to account struct (#1103)
  Allow validators to use any valid combination of gold commitments as stake (#885)
  Fix blockscout websocket jsonrpc url (#1096)
  [Wallet] Preliminary iOS support (#1098)
  [Wallet] Set security fee description translation in Spanish (#1097)
  Exclude generated in vscode file watcher setting (#1082)
  Update .env and .env.integration files (#1087)
  Allow a testnet to run without ethstats (#1085)
  Collect exchange rate time series using notification service (#1020)
  Return to preview view when Fee Education is closed (#1068)
  [Wallet] Pin Setup Flow v2 (#1054)
  Added a variable for electoral threshold (#1023)
  [celotool]Store .env config on GCS after deployment (#1086)
  Group size limit (#1035)
  Fix governance unit tests (#1084)
  Add getExchangeRate to ContractKit (#1083)
  [CLI]Unlock till the geth exits (#1070)
  Add Quorum and Refactor Governance (#430)
  Shuffle elected validators using block randomness (#1033)
  ...
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.

Devs SBAT run an iOS wallet and sync to network
4 participants