[LW-10194] unify codebase of the onboarding and multi-wallet flows #1140

szymonmaslowski · 2024-05-08T08:59:07Z

Checklist

JIRA - <link>
Proper tests implemented
Screenshots added.

Proposed solution

Adjust the multi-wallet code to be generic and able to be used for both multi-wallet and onboarding flows.
This PR creates some amount of dead code which will be removed in the subsequent cleanup PR.

Testing

All onboarding, multi-wallet and forgot password should work as exected without any change in a behavior.

Screenshots

Attach screenshots here if implementation involves some UI changes

github-actions · 2024-05-08T09:19:41Z

Allure Report

allure-report-publisher generated test report!

smokeTests: ✅ test report for bc54849e

	passed	failed	skipped	flaky	total	result
Total	30	0	0	0	30	✅

shawnbusuttil · 2024-05-08T11:57:24Z

apps/browser-extension-wallet/src/providers/AnalyticsProvider/analyticsTracker/events.ts

@@ -69,7 +69,7 @@ export const postHogMultiWalletActions: PostHogMultiWalletActionsType = {
    WALLET_ADDED: PostHogAction.MultiWalletRestoreAdded,
    HD_WALLET: PostHogAction.MultiWalletRestoreHdWallet
  },
-  hw: {
+  hardware: {


shawnbusuttil · 2024-05-08T11:57:40Z

apps/browser-extension-wallet/src/providers/AnalyticsProvider/analyticsTracker/types.ts

@@ -39,7 +39,7 @@ export enum TxCreationType {
 }

 export type OnboardingFlows = 'create' | 'restore' | 'hw' | 'forgot_password' | 'onboarding';


Can you do the same here with the naming?

I want to avoid unnecessary changes in this PR. Once the old onboarding implementation is removed this can be renamed - next PR

shawnbusuttil · 2024-05-08T11:59:33Z

...extension-wallet/src/views/browser-view/features/multi-wallet/create-wallet/CreateWallet.tsx

+export const CreateWallet = (): JSX.Element => (
+  <CreateWalletProvider>
+    {({ step }) => (
+      <>


Was there a reason we removed routing?

Two reasons:

I don't see a value in it, but it can lead to problems: if the user refreshes the page while on the second step then he will lack a state from the previous step

Simpler codebase unification I guess...

shawnbusuttil · 2024-05-08T12:01:00Z

...-wallet/src/views/browser-view/features/multi-wallet/hardware-wallet/HardwareWallet.test.tsx

-      <MemoryRouter initialEntries={[walletRoutePaths.newWallet.hardware.connect]}>
-        <HardwareWallet providers={providers as Providers} />
-      </MemoryRouter>
+      <>


What happens if we remove the fragment here?

Please take a look on the test files again 🙏

shawnbusuttil · 2024-05-08T13:15:07Z

.../src/views/browser-view/features/multi-wallet/restore-wallet/steps/RestoreRecoveryPhrase.tsx


 const wordList = wordlists.english;
 const DEFAULT_MNEMONIC_LENGTH = 24;
 const COPY_PASTE_TOOLTIP_URL = `${process.env.FAQ_URL}?question=best-practices-for-using-the-copy-to-clipboard-paste-from-clipboard-recovery-phrase-features`;

 export const RestoreRecoveryPhrase = (): JSX.Element => {
  const { t } = useTranslation();
+  const { forgotPasswordFlowActive, postHogActions } = useWalletOnboarding();


Why get the analytics from hook if you can access them directly?

They differ depending on the flow: onboarding vs multi-wallet so they has to be passed from the above.

We cannot do import { postHogMultiWalletActions, postHogOnboardingActions } ?

I mean this implementation is used now for both flows so events cannot be hardcoded. they are passed here and here

shawnbusuttil · 2024-05-08T13:16:10Z

...ser-extension-wallet/src/views/browser-view/features/wallet-setup/components/WalletSetup.tsx

-    }
+    // delete "forgot_password" related data if user leaves the flow before completing
+    const clearWallet = () => {
+      deleteFromLocalStorage('wallet');


Do we have keys for the magic strings?

Let me check and improve that in a subsequent cleanup PR 😉

...tension-wallet/src/views/browser-view/features/wallet-setup/components/WalletSetupWizard.tsx

shawnbusuttil

Left some comments.

greatertomi

This is some amazing refactoring. Well-done @szymonmaslowski 🚀

Looks generally good to me, but I don't have enough context on the multi-wallet code, would be a good idea to add @lucas-barros or @renanvalentin as a reviewer.

...-wallet/src/views/browser-view/features/multi-wallet/hardware-wallet/ErrorDialog.module.scss

...er-extension-wallet/src/views/browser-view/features/multi-wallet/hardware-wallet/context.tsx

...ension-wallet/src/views/browser-view/features/multi-wallet/hardware-wallet/steps/Connect.tsx

shawnbusuttil

Left a reply. I think I also have an outstanding https://github.com/input-output-hk/lace/pull/1140/files#r1594016659 comment which wasn't addressed.

sonarqubecloud · 2024-05-20T12:59:37Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

szymonmaslowski requested a review from a team as a code owner May 8, 2024 08:59

szymonmaslowski self-assigned this May 8, 2024

shawnbusuttil reviewed May 8, 2024

View reviewed changes

...tension-wallet/src/views/browser-view/features/wallet-setup/components/WalletSetupWizard.tsx Outdated Show resolved Hide resolved

shawnbusuttil reviewed May 8, 2024

View reviewed changes

greatertomi reviewed May 9, 2024

View reviewed changes

szymonmaslowski requested review from shawnbusuttil and greatertomi May 13, 2024 12:07

shawnbusuttil reviewed May 13, 2024

View reviewed changes

greatertomi approved these changes May 14, 2024

View reviewed changes

shawnbusuttil approved these changes May 14, 2024

View reviewed changes

Base automatically changed from feat/LW-10244-port-HW-onboarding-revam-to-multi-wallet to main May 14, 2024 10:44

lucas-barros approved these changes May 14, 2024

View reviewed changes

szymonmaslowski force-pushed the feat/LW-10194-unify-multiwallet-and-onboarding-codebases branch 3 times, most recently from a9fca6a to 786b901 Compare May 15, 2024 15:29

oldGreg5 approved these changes May 17, 2024

View reviewed changes

szymonmaslowski enabled auto-merge (squash) May 20, 2024 10:00

refactor: unify codebase of the onboarding and multi-wallet flows

bc54849

szymonmaslowski force-pushed the feat/LW-10194-unify-multiwallet-and-onboarding-codebases branch from d2009df to bc54849 Compare May 20, 2024 12:58

szymonmaslowski merged commit b613cde into main May 20, 2024
12 checks passed

szymonmaslowski deleted the feat/LW-10194-unify-multiwallet-and-onboarding-codebases branch May 20, 2024 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LW-10194] unify codebase of the onboarding and multi-wallet flows #1140

[LW-10194] unify codebase of the onboarding and multi-wallet flows #1140

szymonmaslowski commented May 8, 2024 •

edited

Loading

github-actions bot commented May 8, 2024 •

edited

Loading

shawnbusuttil May 8, 2024

shawnbusuttil May 8, 2024

szymonmaslowski May 13, 2024

shawnbusuttil May 8, 2024

szymonmaslowski May 13, 2024

shawnbusuttil May 8, 2024

szymonmaslowski May 13, 2024

shawnbusuttil May 8, 2024

szymonmaslowski May 13, 2024

shawnbusuttil May 13, 2024

szymonmaslowski May 14, 2024

shawnbusuttil May 8, 2024

szymonmaslowski May 14, 2024

shawnbusuttil left a comment

greatertomi left a comment

shawnbusuttil left a comment

sonarqubecloud bot commented May 20, 2024

		@@ -39,7 +39,7 @@ export enum TxCreationType {
		}

		export type OnboardingFlows = 'create' \| 'restore' \| 'hw' \| 'forgot_password' \| 'onboarding';

[LW-10194] unify codebase of the onboarding and multi-wallet flows #1140

[LW-10194] unify codebase of the onboarding and multi-wallet flows #1140

Conversation

szymonmaslowski commented May 8, 2024 • edited Loading

Checklist

Proposed solution

Testing

Screenshots

github-actions bot commented May 8, 2024 • edited Loading

Allure Report

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

shawnbusuttil left a comment

Choose a reason for hiding this comment

greatertomi left a comment

Choose a reason for hiding this comment

shawnbusuttil left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented May 20, 2024

Quality Gate passed

szymonmaslowski commented May 8, 2024 •

edited

Loading

github-actions bot commented May 8, 2024 •

edited

Loading