[Wallet] Pin Setup Flow v2 #1054

jmrossy · 2019-09-19T14:09:29Z

Description

Implement new Secure Wallet onboarding designs
Fix custom pincode flow (wasn't setting account password correctly)
Fix navigation to send flow from tab bar icon
Add caching of custom pincodes
Convert pincode thunks to Saga and add tests
Rename Pincode.tsx -> PincodeSet.tsx (and mostly redo)
Rename SystemAuth.tsx -> PincodeEducation.tsx
Add migration for new redux state around pincodeType
Add a Number Keypad component for in-app keypad
Add a backspace icon
Add an obfuscated Pincode text component
Improve error handling of pin setup screens

Tested

Ran through onboarding flow using both phone auth and custom pin
Sent a tx using both phone auth and custom pin
navigated to send flow

Related issues

Backwards compatibility

Yes but I needed to add a redux migration

Fix Send screen tab nav Add saga tests for account

martinvol · 2019-09-19T15:48:07Z

packages/mobile/src/invite/JoinCelo.tsx

@@ -133,7 +131,10 @@ export class JoinCelo extends React.Component<Props, State> {
  }

  nextScreen = () => {
-    const nextScreen = this.props.pincodeSet ? Screens.EnterInviteCode : Screens.Pincode
+    const nextScreen =
+      this.props.pincodeType === PincodeType.Unset


Not completely sure, but I think here you have an edge case when the user does the onboarding flow and then disables the security of their phone (think likely to happen with people that only set up the PIN to use the app). It probably won't explicitly fail here but when they try to send a transaction getPinFromKeystore will just throw an error.

So I'd suggest checking every time ConfirmDeviceCredentials.isDeviceSecure(), specially when sending a transaction.

Did you mean this as a general point or something specific to this line? Because it seems unrelated to the JoinCelo page.

In the case where the user removes phone security after onboarding with it, throwing an error in getPinFromKeystore seems like the right thing to do. Is there an alternative you'd suggest?

As a general point I meant.

Yeah, getting getPinFromKeystore to throw an error is the right call but I think it should be catched so the user can get redirected to the education screen again (maybe with a slightly different copy).

Currently, if getPinFromKeystore throws an error (of any kind), then unlockAccount will catch it, causing getConnectedUnlockedAccount to then throw an IncorrectPin error. I agree it would be good to differentiate incorrect pin from missing phone auth but IIUC it would require modifying everywhere that getConnectedUnlockedAccount is used to handle that error case. Not too bad but still a bit out of scope for this PR I think. WDYT?

Agreed, opened #1090!

jeanregisser

Great! 👍

jeanregisser · 2019-09-23T08:27:44Z

packages/mobile/src/navigator/TabNavigator.tsx

@@ -109,6 +108,9 @@ export const TabNavigator = createBottomTabNavigator(
        tabBarOnPress: () => {
          navigate(Stacks.SendStack)
        },
+        tabBarOnLongPress: () => {
+          navigate(Stacks.SendStack)


jeanregisser · 2019-09-23T08:35:58Z

packages/mobile/src/pincode/PincodeCache.ts

+  if (
+    pincodeCache.pincode &&
+    pincodeCache.timestamp &&
+    Date.now() - pincodeCache.timestamp < PIN_TIMEOUT


nit: this could be bypassed by changing the system time. Would be better to use a monotonic clock. https://stackoverflow.com/questions/7272395/monotonically-increasing-time-in-javascript

Will add a TODO for this

jeanregisser · 2019-09-23T08:51:35Z

packages/mobile/src/redux/migrations.test.ts

@@ -31,4 +32,17 @@ describe('Redux persist migrations', () => {
    const migratedSchema = migrations[1](v0Stub)
    expect(migratedSchema.app.language).toEqual('es-419')
  })
+
+  it('work for v1 to v2', () => {
+    const defaultSchema = getLatestSchema()


It looks like it should take v1Schema here instead.

Fixed in this and the other tests :)

jeanregisser · 2019-09-23T08:53:27Z

packages/mobile/src/redux/migrations.ts

+      ...state,
+      account: {
+        ...state.account,
+        pincodeType: state.account.pincodeSet ? PincodeType.PhoneAuth : PincodeType.Unset,


Though it's harmless, we should clean up the old property by adding pincodeSet: undefined.

Good idea, done

jeanregisser · 2019-09-23T09:05:22Z

packages/react-components/components/NumberKeypad.tsx

+      <View style={style.row}>
+        {props.showDecimal ? (
+          <Touchable borderless={true} onPress={props.onDecimalPress}>
+            <Text style={style.digit}>.</Text>


This will be incorrect for some locales.

Yep, I haven't i18n'ed this yet, will add a TODO

jeanregisser · 2019-09-23T09:11:09Z

packages/react-components/components/NumberKeypad.tsx

+  )
+}
+
+export default function NumberKeypad(props: Props) {


I usually don't like custom keyboards as they break my expectations (can't use integration with password managers, or swipe style input).
Sure we really want our own? I understand it's for security reasons, but if the system keyboard is compromised I'm not entirely convinced a custom keypad will actually prevent much.

Also, do custom keyboards have any trouble on low end phones?

A lot of apps use custom keyboards for digits because the android numeric keyboard type varies so much across different phone vendors. And many look terrible.

@annakaz not sure why this component would cause problems on low end phones. If anything the experience is better because the component doesn't resize/redraw as the keyboard pops in and out

codecov · 2019-09-24T02:00:44Z

Codecov Report

Merging #1054 into master will decrease coverage by 0.49%.
The diff coverage is 54.16%.

@@            Coverage Diff            @@
##           master    #1054     +/-   ##
=========================================
- Coverage   66.94%   66.45%   -0.5%     
=========================================
  Files         252      256      +4     
  Lines        7322     7349     +27     
  Branches      415      424      +9     
=========================================
- Hits         4902     4884     -18     
- Misses       2334     2377     +43     
- Partials       86       88      +2

Flag	Coverage Δ
#mobile	`66.45% <54.16%> (-0.5%)`	⬇️

Impacted Files	Coverage Δ
packages/mobile/src/redux/store.ts	`96% <ø> (ø)`	⬆️
packages/mobile/src/language/Language.tsx	`74.07% <ø> (+1.66%)`	⬆️
packages/mobile/src/backup/BackupIntroduction.tsx	`73.33% <ø> (ø)`	⬆️
packages/mobile/src/web3/actions.ts	`81.48% <ø> (+3.43%)`	⬆️
packages/mobile/src/import/ImportContacts.tsx	`75% <ø> (ø)`	⬆️
packages/mobile/src/config.ts	`94.11% <ø> (-0.33%)`	⬇️
packages/mobile/src/navigator/Navigator.tsx	`0% <0%> (ø)`	⬆️
packages/mobile/src/navigator/TabNavigator.tsx	`0% <0%> (ø)`	⬆️
packages/mobile/src/pincode/PincodeEducation.tsx	`0% <0%> (ø)`
...ackages/mobile/src/pincode/PincodeUtils.android.ts	`0% <0%> (ø)`
... and 27 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 f80e93d...0174b74. Read the comment docs.

* 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) ...

jmrossy added 6 commits September 17, 2019 17:05

in progress

5593ebe

Merge branch 'master' into rossy/wa-security-choice

4c71ab8

UI work done

916a107

Merge branch 'master' into rossy/wa-security-choice

74d8bd9

Finish pincode re-implementation

b445bad

Fix Send screen tab nav Add saga tests for account

Fix store version

ad497f4

jmrossy requested review from jeanregisser, martinvol, cmcewen, Pedro-vk and annakaz September 19, 2019 14:09

jmrossy assigned jeanregisser, martinvol, cmcewen, Pedro-vk and annakaz Sep 19, 2019

martinvol reviewed Sep 19, 2019

View reviewed changes

jeanregisser approved these changes Sep 23, 2019

View reviewed changes

annakaz removed their assignment Sep 23, 2019

ashishb closed this Sep 24, 2019

ashishb deleted the rossy/wa-security-choice branch September 24, 2019 01:28

ashishb restored the rossy/wa-security-choice branch September 24, 2019 01:36

ashishb reopened this Sep 24, 2019

jmrossy added 4 commits September 24, 2019 12:42

Address feedback

3603687

Merge branch 'master' into rossy/wa-security-choice

d3c319b

Fix saga test

1384579

Merge branch 'master' into rossy/wa-security-choice

0174b74

jmrossy force-pushed the rossy/wa-security-choice branch from adfa903 to 0174b74 Compare September 24, 2019 11:49

martinvol mentioned this pull request Sep 24, 2019

[Wallet] Users SBAT send transactions even after disabling security on their phone #1090

Closed

jmrossy merged commit 34a29d9 into master Sep 24, 2019

jmrossy deleted the rossy/wa-security-choice branch September 24, 2019 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Wallet] Pin Setup Flow v2 #1054

[Wallet] Pin Setup Flow v2 #1054

jmrossy commented Sep 19, 2019 •

edited

Loading

martinvol Sep 19, 2019 •

edited

Loading

jmrossy Sep 19, 2019

martinvol Sep 19, 2019

jmrossy Sep 20, 2019 •

edited

Loading

martinvol Sep 24, 2019

jeanregisser left a comment

jeanregisser Sep 23, 2019

jeanregisser Sep 23, 2019

jmrossy Sep 24, 2019

jeanregisser Sep 23, 2019

jmrossy Sep 24, 2019

jeanregisser Sep 23, 2019

jmrossy Sep 24, 2019

jeanregisser Sep 23, 2019

jmrossy Sep 24, 2019

jeanregisser Sep 23, 2019

annakaz Sep 23, 2019

jmrossy Sep 24, 2019

codecov bot commented Sep 24, 2019 •

edited

Loading

[Wallet] Pin Setup Flow v2 #1054

[Wallet] Pin Setup Flow v2 #1054

Conversation

jmrossy commented Sep 19, 2019 • edited Loading

Description

Tested

Related issues

Backwards compatibility

martinvol Sep 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrossy Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 24, 2019 • edited Loading

Codecov Report

jmrossy commented Sep 19, 2019 •

edited

Loading

martinvol Sep 19, 2019 •

edited

Loading

jmrossy Sep 20, 2019 •

edited

Loading

codecov bot commented Sep 24, 2019 •

edited

Loading