WIP: LIVE-1792 - Onboarding #2510

alexandremgo · 2022-05-13T12:29:45Z

New onboarding flow. A video 📹 is available on channel live-803

Type

Feature. Only the logic, should not be merged.

Context

PR open for comments, for ticket LIVE-1792.

Dependencies

This branch depends on this branch of live-common

- polling mechanism to fetch the device state - bluetooth pairing during onboarding - navigator to go to the onboarding screen

And implement strategy to only pass serializable values in params

juan-cortes

🤓 a few remarks

juan-cortes · 2022-05-17T09:07:32Z

src/locales/en/common.json

@@ -884,6 +884,7 @@
      "nanoS": "Nano S",
      "nanoSP": "Nano S Plus",
      "nanoX": "Nano X",
+      "nanoDev": "Nano Dev",


Can we call this Nano FTS for consistency, even if it means nothing?

juan-cortes · 2022-05-17T09:17:15Z

src/screens/PairDevices/index.js

+            from(getVersion(t)),
+          ).toPromise();
+        } catch (_) {
+          // Silently swwallowing error, we only want to trigger the native ble pairing step


Can't we extract this into a hook that does the callback for us instead of having it inline? I don't like the whole try/catch there in the middle of the pairing screen if that makes sense.

const exitEarly = useHook(mySuperAwesomeHook(onDone, device, route.params)) if (route.params?.onlySelectDeviceWithoutFullAppPairing && exitEarly) return

Also, what if the user rejects though? Wouldn't this also be considered a successful pairing by this logic?
We need to make sure that the thrown error below is not of the user refused or pairing unsuccessful types

juan-cortes · 2022-05-17T09:25:34Z

src/screens/Onboarding/steps/deviceSelection.tsx

-      deviceModelId,
-    });
+    if (deviceModelId === "nanoDev") {
+      // TODO: fix navigation typescript by adding a type def RootStackParamList to the createStackNavigator


No idea what you mean here, but seems like you do, so why not fix it

Fixing it means to type i would say at least BaseOnboardingNavigator (and probably BaseNavigator, but not sure of the current type of navigation because we use useNavigation() just above) like i did with SyncOnboardingNavigator

So it would probably be better to do it in another PR that add stuff to this one IMO

juan-cortes · 2022-05-17T09:50:09Z

src/screens/PairDevices/index.js

-      route.params?.onDone?.(device);
+      // To avoid passing a onDone function param that is not serializable
+      if (route.params?.onDoneNavigateTo === ScreenName.SyncOnboardingWelcome) {
+        console.log("PairDevices: 🦮 navigate directly to SyncOnboarding");


I like dogs

Not sure I follow what's happening though

It's to avoid passing a onDone function as a route argument. It should be avoided according to the doc of react-navigation. To be sure that the navigation flow works correctly, we should only pass serialisable params

juan-cortes · 2022-05-17T09:56:09Z

src/screens/SyncOnboarding/index.tsx

+import { ScreenName } from "../../const";
+import { SyncOnboardingStackParamList } from "../../components/RootNavigator/SyncOnboardingNavigator";
+
+// FIXME: Define an initial onboarding state - or cannot have an initial state in our use case ?


It depends on whether you are showing anything before the first successful polling? Perhaps not having a state until we build it from the flags is also not a bad idea 🦐

juan-cortes · 2022-05-17T09:58:25Z

src/screens/SyncOnboarding/index.tsx

+  isInRecoveryMode: false,
+  isRecoveringSeed: false,
+  isConfirmingSeedWords: false,
+  seedPhraseType: "24-words",


Makes me think, have you tested it with 12 words too? If it's a type, can't we make it an enum so that we are not playing with strings here? SeedType.TwentyFour

In typescript i would stick with a union type of string literals (like i did for SeedPhraseType):

a string enum (ex enum SeedType { TwentyFour: "24-words"}) is transpiled in JS to a weird checking function

a numeric enum (ex enum SeedType { TwentyFour }) when debugging will only be numbers

you can't extend an enum (even if in our case of SeedType we don't need it) without modifying its values

I feel the advantages of union types of string literals over enums are bigger than the drawbacks

But it's true that this is really a TypeScript thing. I would say we should all use the same (either enums, either union of string literals)

WDYT ?

And yeah, i tested with 12 and 18 words ;) i put the value "24-words" as the initial state because i'm still not sure what to do with the initial state ahah (maybe i should allow for the OnboardingState a seedPhraseType of type SeedPhraseType | null ? (here)

But IMO it's better that we all follow the same convention. As there is enum for DeviceModelId it's maybe better to go with enum (or enum for stuff we know won't change / won't need to be extended ?) ?

(but still think that string literals are better in TS 🙈

At the end i've used enum because the SeedPhraseType is fixed and will not be extended by another type + union of string literals in TS (at least in the current version of TS we have) needs to add a as SeedPhraseType (for ex "24-words as SeedPhraseType when using them in another context (in LLM for ex), so i needed to import SeedPhraseType and it ended up being annoying

juan-cortes · 2022-05-17T10:08:22Z

src/screens/SyncOnboarding/index.tsx

+            console.log(`SyncOnboarding: ▶️ Polling ${i}`);
+          }),
+          concatMap(() =>
+            withDevice(device.deviceId)(t => from(getVersion(t))),


Can't we simplify this greatly with

withDevice(deviceId)((t => from(getVersion(t))).pipe(retryWhen(retryWhileErrors(acceptError)))

Where acceptError does something like this.

juan-cortes · 2022-05-17T10:08:29Z

src/screens/SyncOnboarding/index.tsx

+      onboardingStatePollingSubscription = timer(0, pollingFrequencyMs)
+        .pipe(
+          tap(i => {
+            console.log(`SyncOnboarding: ▶️ Polling ${i}`);
+          }),
+          concatMap(() =>
+            withDevice(device.deviceId)(t => from(getVersion(t))),
+          ),
+          retryWhen(errors =>
+            errors.pipe(
+              mergeMap(error => {
+                // Transport error: retry polling
+                if (
+                  error &&
+                  error instanceof TransportStatusError &&
+                  // @ts-expect-error TransportStatusError is not a class
+                  error.statusCode === 0x6d06
+                ) {
+                  console.log(
+                    `SyncOnboarding: 0x6d06 error 🔨 ${JSON.stringify(error)}`,
+                  );
+                  return of(error);
+                }
+                // Disconnection error: retry polling
+                if (
+                  error &&
+                  error instanceof Error &&
+                  error.name === "DisconnectedDevice"
+                ) {
+                  console.log(
+                    `SyncOnboarding: disconnection error 🔌 ${JSON.stringify(
+                      error,
+                    )}`,
+                  );
+                  return of(error);
+                }
+
+                console.log(
+                  `SyncOnboarding: 💥 Error ${error} -> ${JSON.stringify(
+                    error,
+                  )}`,
+                );
+                return throwError(error);
+              }),
+              tap(() => console.log("Going to retry in 🕐️ ...")),
+              delayWhen(() => timer(pollingFrequencyMs)),
+              tap(() => console.log("Retrying 🏃️ !")),
+            ),
+          ),
+          map((deviceVersion: FirmwareInfo) =>
+            extractOnboardingState(deviceVersion.flags),
+          ),
+        )


Definitely move this to live-common

juan-cortes · 2022-05-17T10:11:21Z

src/screens/SyncOnboarding/index.tsx

+  // When reaching the synchronized onboarding, the device could already
+  // be BLE paired to the phone and at the same time not known by LLM,
+  // it does not matter.
+  // So as long as the device is not onboarded, the LLM will ask for a pairing.


It will technically not ask for a pairing but rather send the getVersion from https://github.com/LedgerHQ/ledger-live-mobile/pull/2510/files#diff-e4fd4347723086bdb5b2f409bfa15efd6ae4eb4ba7f7fc051c7edb8fe4383ceeR129 and actually work.

alexandremgo added 6 commits May 10, 2022 09:07

chore: access onboarding state extract feature from llc

b27c2d1

feat: sync onboarding logic and screen

f672b35

- polling mechanism to fetch the device state - bluetooth pairing during onboarding - navigator to go to the onboarding screen

feat: WIP select a nano for sync onboarding

20c63e1

fix(SyncOnboarding): retry strategy on errors while polling

15af348

feat(SyncOnboarding): Typed navigator for SyncOnboarding

ee56996

And implement strategy to only pass serializable values in params

feat(deeplink): enable deep linking to onboarding steps

917e6dd

alexandremgo requested review from a team as code owners May 13, 2022 12:29

alexandremgo marked this pull request as draft May 13, 2022 12:30

alexandremgo requested a review from juan-cortes May 13, 2022 12:32

feat(SyncOnboarding): handle disconnected device

cddfb33

alexandremgo force-pushed the feat/LIVE-1792 branch from 939e7c2 to cddfb33 Compare May 13, 2022 16:03

alexandremgo added 2 commits May 16, 2022 15:09

fix(SyncOnboarding): trigger phone pairing before moving to onboarding

19fc643

fix(SyncOnboarding): use theme colors

81e9c48

juan-cortes suggested changes May 17, 2022

View reviewed changes

alexandremgo mentioned this pull request May 30, 2022

WIP: Live 1792: Synchronized onboarding logic LedgerHQ/ledger-live#214

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: LIVE-1792 - Onboarding #2510

WIP: LIVE-1792 - Onboarding #2510

alexandremgo commented May 13, 2022

juan-cortes left a comment

juan-cortes May 17, 2022

juan-cortes May 17, 2022

juan-cortes May 17, 2022

juan-cortes May 17, 2022

juan-cortes May 17, 2022

alexandremgo May 19, 2022

alexandremgo May 19, 2022

juan-cortes May 17, 2022

juan-cortes May 17, 2022

alexandremgo May 19, 2022

juan-cortes May 17, 2022

juan-cortes May 17, 2022

alexandremgo May 19, 2022 •

edited

alexandremgo May 20, 2022

alexandremgo May 30, 2022

juan-cortes May 17, 2022

juan-cortes May 17, 2022

juan-cortes May 17, 2022

WIP: LIVE-1792 - Onboarding #2510

Are you sure you want to change the base?

WIP: LIVE-1792 - Onboarding #2510

Conversation

alexandremgo commented May 13, 2022

Type

Context

Dependencies

juan-cortes 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

alexandremgo May 19, 2022 • edited

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

alexandremgo May 19, 2022 •

edited