feat: new mobile wallet create flow #8760

NeOMakinG · 2025-02-06T15:49:53Z

Description

Implement the new mobile flow of creating a wallet, backing up mnemonic

Using the dialog component

As a follow up, we should replace this flow from the splash screen too

Issue (if applicable)

closes #8706

Risk

High (create wallet on mobile, mnemonic memory leaks...)

High Risk PRs Require 2 approvals

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Testing

Create a wallet using the mobile app (desktop and responsive shouldn't change yet)
Create wallet from the splash screen mobile app didn't change yet (because the drawer is not initialized on this part yet)
Try to select a wallet and hit backup, the mobile should ask for the password/faceid and show the right backup phrase, next button shouldn't be shown

Engineering

A small concern on memory clean up that I'll add as a self-review comment

Operations

n.a

🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

https://jam.dev/c/0e88ee55-607f-485a-a705-deb03c002e53

Pushed to gome so we can test!

src/pages/Dashboard/components/DashboardHeader/DashboardHeaderTop.tsx

NeOMakinG · 2025-02-07T10:50:48Z

src/components/MobileWalletDialog/routes/DeleteWallet/Backup.tsx

It was a legacy component from the initial dialog implementation, not used anymore

NeOMakinG · 2025-02-07T10:51:33Z

src/components/MobileWalletDialog/routes/CreateWallet/ManualBackup.tsx

Move this file to it's proper folder, this is a feature by itself as we also redirect to this view one level upper in the tree (when backing up already created wallet)

NeOMakinG · 2025-02-07T10:51:54Z

src/components/CarouselDots/CarouselDots.tsx

@@ -18,10 +24,12 @@ export const CarouselDots = ({ length, activeIndex, onClick }: CarouselDotsProps
          width='7px'
          height='7px'
          borderRadius='50%'
-          backgroundColor={i === activeIndex - 1 ? activeDotBackgroundColor : 'text.subtle'}
+          backgroundColor={
+            i === activeIndex - 1 ? activeDotBackgroundColor : inactiveDotBackgroundColor


Make it more customizable anywhere

NeOMakinG · 2025-02-07T10:54:05Z

src/components/MobileWalletDialog/routes/CreateWallet/CreateWallet.tsx

+  useEffect(() => {
+    try {
+      if (!vault) setVault(location.state?.vault ?? createWallet())
+    } catch (e) {
+      console.log(e)
+    }


My biggest concern:

Should we revoke the wallet and recreate it in every screens? It's a lot of boilerplate/verbose code that I avoided, because I mean, if a not-memoized component is unmounted, it shouldn't stay in memory

But happy to bring it if we feel like we need some extra safety

Think this should be safe indeed since this is a state var.
Revoker basically just revokes whichever secret (seed, master key, isolated mnemonic or whatever the isolation wallet does magic-wise...), but indeed, vault is also nuked from cache on unmount in theory.

With that being said,I'm not sure that it being theoretically safe is a good enough reason not to revoke, especially given how routing works (definitely seen many instances of routes still being mounted), and not super confident with security relying on such assumption.

@NeOMakinG can we keep it non-revoked and assume routes being unmounted means garbage collect kicking in and nuking vault for that PR but do a direct follow-up with revoke() boilerplate?

gomesalexandre

What a beast of a PR ser. Functionally looks sane minus a few smolish comments, runtime test incoming.

gomesalexandre · 2025-02-07T21:07:47Z

src/components/MobileWalletDialog/MobileWalletDialog.tsx

@@ -2,6 +2,8 @@ import { AnimatePresence } from 'framer-motion'
 import { MemoryRouter, Redirect, Route, Switch } from 'react-router'
 import { Dialog } from 'components/Modal/components/Dialog'

+import { CreateRouter } from './routes/CreateWallet/CreateRouter'


nitpick: CreateWalletRouter

gomesalexandre · 2025-02-07T21:08:09Z

src/components/MobileWalletDialog/routes/SavedWallets.tsx

@@ -87,7 +101,7 @@ export const SavedWallets: React.FC<SavedWalletsProps> = ({ onClose }) => {
          variant='ghost'
          colorScheme='blue'
          leftIcon={addIcon}
-          onClick={handleCreate}
+          onClick={handleCreateClick}


gomesalexandre · 2025-02-07T21:41:42Z

src/components/MobileWalletDialog/routes/CreateWallet/CreateBackupConfirm.tsx

+        setSelectedWordIndex(prev => {
+          const next = (prev ?? -1) + 1
+          if (next >= TEST_COUNT_REQUIRED) {
+            ;(async () => {


q: should this method be an async callback instead of IIAFE?

gomesalexandre · 2025-02-07T21:49:41Z

src/components/MobileWalletDialog/routes/CreateWallet/CreateWallet.tsx

+  useEffect(() => {
+    try {
+      if (!vault) setVault(location.state?.vault ?? createWallet())
+    } catch (e) {
+      console.log(e)
+    }


Think this should be safe indeed since this is a state var.
Revoker basically just revokes whichever secret (seed, master key, isolated mnemonic or whatever the isolation wallet does magic-wise...), but indeed, vault is also nuked from cache on unmount in theory.

With that being said,I'm not sure that it being theoretically safe is a good enough reason not to revoke, especially given how routing works (definitely seen many instances of routes still being mounted), and not super confident with security relying on such assumption.

@NeOMakinG can we keep it non-revoked and assume routes being unmounted means garbage collect kicking in and nuking vault for that PR but do a direct follow-up with revoke() boilerplate?

gomesalexandre · 2025-02-07T21:50:26Z

src/components/MobileWalletDialog/routes/CreateWallet/ManualBackup.tsx

+
+          <Box bg={bgColor} borderRadius='xl' p={4} width='full'>
+            <SimpleGrid columns={2} spacing={2}>
+              {words.map((word, index) => (


nitpick: can be memoized

gomesalexandre

Tested locally, confirmed we're still looking good create (in splash and in wallet-connected state) and backup-wise (on new create backup state as well as in wallet-connected state).

Some smallish visual shenanigans to be tackled, though nothing major blocking functionally.

Manual backup hints that there is a next step, however showContinueButton is false when in the context of a connected wallet we're backing up, and this copy should probably be hidden?

Similarly re: flow not being adapter to the "backing up a connected wallet" flow and assuming this is the backup step when creating a wallet, there is a stepper which is also confusing and probably shouldn't be here

Seed looks sane above ✅
Creating a wallet is sane ✅

One lil weird flash of testing a word at around 51 seconds if we want to tackle this @NeOMakinG https://jam.dev/c/566a5b5a-5320-4057-a362-6750a886a775?startFrom=51.70

Creating a wallet from splash is still happy ✅

https://jam.dev/c/a5507c51-4732-4538-a9a8-630fa8cc60e0

feat: new mobile wallet create flow

ae16be2

NeOMakinG commented Feb 6, 2025

View reviewed changes

src/pages/Dashboard/components/DashboardHeader/DashboardHeaderTop.tsx Outdated Show resolved Hide resolved

fix: finish

7c200bd

NeOMakinG commented Feb 7, 2025

View reviewed changes

fix: remove commented code

ba5a39d

NeOMakinG marked this pull request as ready for review February 7, 2025 10:58

NeOMakinG requested a review from a team as a code owner February 7, 2025 10:58

gomesalexandre self-requested a review February 7, 2025 20:21

gomesalexandre reviewed Feb 7, 2025

View reviewed changes

gomesalexandre approved these changes Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new mobile wallet create flow #8760

feat: new mobile wallet create flow #8760

NeOMakinG commented Feb 6, 2025 •

edited

Loading

NeOMakinG Feb 7, 2025

NeOMakinG Feb 7, 2025

NeOMakinG Feb 7, 2025

NeOMakinG Feb 7, 2025

gomesalexandre Feb 7, 2025

gomesalexandre left a comment

gomesalexandre Feb 7, 2025

gomesalexandre Feb 7, 2025

gomesalexandre Feb 7, 2025

gomesalexandre Feb 7, 2025

gomesalexandre Feb 7, 2025

gomesalexandre left a comment

feat: new mobile wallet create flow #8760

Are you sure you want to change the base?

feat: new mobile wallet create flow #8760

Conversation

NeOMakinG commented Feb 6, 2025 • edited Loading

Description

Issue (if applicable)

Risk

Testing

Engineering

Operations

Screenshots (if applicable)

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

gomesalexandre 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

gomesalexandre left a comment

Choose a reason for hiding this comment

NeOMakinG commented Feb 6, 2025 •

edited

Loading