fix: issues found during review

lidofinance · Sep 21, 2023 · e77ffe7 · e77ffe7
1 parent a80719e
commit e77ffe7
Show file tree

Hide file tree

Showing 8 changed files with 81 additions and 95 deletions.
diff --git a/apps/demo-react/pages/index.tsx b/apps/demo-react/pages/index.tsx
@@ -29,11 +29,9 @@ export function Web() {
   const [selectedTheme, setSelectedTheme] = useState('light' as ThemeName);
 
   const isSelectedThemeLight = selectedTheme === ThemeName.light;
-  if (isSelectedThemeLight) {
-    document.documentElement.dataset.lidoTheme = 'light';
-  } else {
-    document.documentElement.dataset.lidoTheme = 'dark';
-  }
+  document.documentElement.dataset.lidoTheme = isSelectedThemeLight
+    ? 'light'
+    : 'dark';
 
   return (
     <ThemeProvider theme={isSelectedThemeLight ? themeLight : themeDark}>

diff --git a/packages/connect-wallet-modal/src/components/Ledger/LedgerAccountButton.tsx b/packages/connect-wallet-modal/src/components/Ledger/LedgerAccountButton.tsx
@@ -6,7 +6,7 @@ import { AccountRecord } from './types';
 
 const ButtonStyled = styled.div`
   width: 100%;
-  background-color: ${({ theme }) => theme.colors.backgroundSecondary};
+  background-color: var(--lido-color-backgroundSecondary);
   border-radius: 10px;
   display: flex;
   padding: 16px 20px;
@@ -21,7 +21,7 @@ const ButtonStyled = styled.div`
 
   :hover {
     cursor: pointer;
-    background-color: ${({ theme }) => theme.colors.backgroundDarken};
+    background-color: var(--lido-color-backgroundDarken);
   }
 `;
 
@@ -48,10 +48,10 @@ const gradientKeyframes = keyframes`
 const SkeletonWrapper = styled(ButtonStyled)`
   background: linear-gradient(
       270deg,
-      ${({ theme }) => theme.colors.backgroundSecondary} 0%,
-      ${({ theme }) => theme.colors.backgroundSecondary} 0.01%,
-      ${({ theme }) => theme.colors.foreground} 34.14%,
-      ${({ theme }) => theme.colors.backgroundSecondary} 100%
+      var(--lido-color-backgroundSecondary) 0%,
+      var(--lido-color-backgroundSecondary) 0.01%,
+      var(--lido-color-foreground) 34.14%,
+      var(--lido-color-backgroundSecondary) 100%
     )
     0 0;
   animation: ${gradientKeyframes} 2s ease infinite;

diff --git a/packages/connect-wallet-modal/src/components/Ledger/LedgerAccountScreen.tsx b/packages/connect-wallet-modal/src/components/Ledger/LedgerAccountScreen.tsx
@@ -59,18 +59,18 @@ export const LedgerAccountScreen: FC<Props> = ({ metrics, closeScreen }) => {
 
   const onConnectLedger = metrics?.events?.connect?.handlers.onConnectLedger;
   const { connect, connector } = useConnectorLedger({
-    onConnect: () => {
-      onConnectLedger?.();
-    },
+    onConnect: onConnectLedger,
   });
 
-  if (!connect || !connector?.isSupported()) {
-    setError(
-      new Error(
-        "Your browser doesn't support direct connection with Ledger. Please, try another browser.",
-      ),
-    );
-  }
+  useEffect(() => {
+    if (!connect || !connector?.isSupported()) {
+      setError(
+        new Error(
+          "Your browser doesn't support direct connection with Ledger. Please, try another browser.",
+        ),
+      );
+    }
+  }, [connect, connector, setError]);
 
   const handleAccountButtonClick = useCallback(
     async (account) => {
@@ -92,34 +92,33 @@ export const LedgerAccountScreen: FC<Props> = ({ metrics, closeScreen }) => {
       <StackItem>
         <LedgerDerivationPathSelect
           value={derivationPathTemplate}
-          onChange={(value) => {
-            handleDerivationPathSelect(value);
-          }}
+          onChange={handleDerivationPathSelect}
         />
       </StackItem>
       <StackItem>
         {/* If accounts storage contains the first account for the current page, we can assume that others are loaded too */}
-        {accountsStorage?.[derivationPathTemplate]?.[
-          getFirstIndexOnPage(currentPage, ACCOUNTS_PER_PAGE)
-        ]
-          ? Array.from({ length: ACCOUNTS_PER_PAGE }).map((_, i) => {
-              const accountIndex =
-                i + getFirstIndexOnPage(currentPage, ACCOUNTS_PER_PAGE);
-              const account =
-                accountsStorage[derivationPathTemplate][accountIndex];
-              return (
-                <AccountButton
-                  key={account.address}
-                  {...account}
-                  onClick={() => {
-                    void handleAccountButtonClick(account);
-                  }}
-                />
-              );
-            })
-          : Array.from({ length: ACCOUNTS_PER_PAGE }).map((_, i) => (
-              <AccountButtonSkeleton key={i} />
-            ))}
+        {Array.from({ length: ACCOUNTS_PER_PAGE }).map(
+          accountsStorage?.[derivationPathTemplate]?.[
+            getFirstIndexOnPage(currentPage, ACCOUNTS_PER_PAGE)
+          ]
+            ? (_, i) => {
+                const accountIndex =
+                  i + getFirstIndexOnPage(currentPage, ACCOUNTS_PER_PAGE);
+                const account =
+                  accountsStorage[derivationPathTemplate][accountIndex];
+                return (
+                  <AccountButton
+                    key={account.address}
+                    {...account}
+                    onClick={() => {
+                      void handleAccountButtonClick(account);
+                    }}
+                  />
+                );
+              }
+            : (_, i) => <AccountButtonSkeleton key={i} />,
+        )}
+
         <BoxWrapper>
           <Pagination
             activePage={currentPage}

diff --git a/packages/connect-wallet-modal/src/components/Ledger/LedgerContext.tsx b/packages/connect-wallet-modal/src/components/Ledger/LedgerContext.tsx
@@ -41,11 +41,15 @@ export const LedgerContextProvider: FC<LedgerContextProps> = ({
     useState(0);
 
   const disconnectTransport = useCallback(async (goingToReconnect = false) => {
-    await transport?.current?.close();
-    transport.current = null;
-    ledgerAppEth.current = null;
-    if (!goingToReconnect) {
-      setIsTransportConnected(false);
+    try {
+      await transport?.current?.close();
+      transport.current = null;
+      ledgerAppEth.current = null;
+      if (!goingToReconnect) {
+        setIsTransportConnected(false);
+      }
+    } catch (e: any) {
+      setError(interceptLedgerError(e));
     }
   }, []);
 
@@ -69,15 +73,10 @@ export const LedgerContextProvider: FC<LedgerContextProps> = ({
   }, [disconnectTransport, connectTransport]);
 
   useEffect(() => {
-    if (isActive) {
-      void connectTransport();
-    }
+    if (!isActive) return;
+    void connectTransport();
 
-    return () => {
-      if (isActive) {
-        void transport?.current?.close();
-      }
-    };
+    return () => void transport?.current?.close();
   }, [connectTransport, disconnectTransport, isActive]);
 
   const value = useMemo(

diff --git a/packages/connect-wallet-modal/src/components/Ledger/LedgerDerivationPathSelect.tsx b/packages/connect-wallet-modal/src/components/Ledger/LedgerDerivationPathSelect.tsx
@@ -4,7 +4,7 @@ import styled from '@reef-knot/ui-react/styled-wrapper';
 import { DERIVATION_PATHS } from './constants';
 
 const TextStyled = styled.p`
-  color: ${({ theme }) => theme.colors.textSecondary};
+  color: var(--lido-color-textSecondary);
   font-size: ${({ theme }) => theme.fontSizesMap.xs}px;
   line-height: 1.7;
   text-align: left;

diff --git a/packages/connect-wallet-modal/src/components/Ledger/LedgerModal.tsx b/packages/connect-wallet-modal/src/components/Ledger/LedgerModal.tsx
@@ -24,11 +24,7 @@ export const LedgerModal = (props: LedgerModalProps) => {
   );
 
   return (
-    <Modal
-      title="Ledger connect"
-      {...props}
-      onClose={handleClose}
-    >
+    <Modal title="Ledger connect" {...props} onClose={handleClose}>
       <LedgerContextProvider isActive={!!props.open}>
         <LedgerScreen {...props} />
       </LedgerContextProvider>
@@ -42,16 +38,16 @@ export const LedgerScreen = ({ metrics, onClose }: LedgerModalProps) => {
 
   return (
     <LedgerModalInnerContainer>
-      {error ? (
+      {error && (
         <LedgerErrorScreen
           message={error.message}
           retry={() => void reconnectTransport()}
         />
-      ) : isTransportConnected ? (
+      )}
+      {!error && isTransportConnected && (
         <LedgerAccountScreen metrics={metrics} closeScreen={onClose} />
-      ) : (
-        <LedgerConnectionScreen />
       )}
+      {!error && !isTransportConnected && <LedgerConnectionScreen />}
     </LedgerModalInnerContainer>
   );
 };
diff --git a/packages/connect-wallet-modal/src/components/Ledger/helpers.ts b/packages/connect-wallet-modal/src/components/Ledger/helpers.ts
@@ -2,9 +2,8 @@ import { LS_KEY_DERIVATION_PATH } from '@reef-knot/ledger-connector';
 import { LedgerContextValue } from './LedgerContext';
 import { DERIVATION_PATHS } from './constants';
 
-export async function getTransport() {
-  return await (await import('@ledgerhq/hw-transport-webhid')).default.create();
-}
+export const getTransport = async () =>
+  (await import('@ledgerhq/hw-transport-webhid')).default.create();
 
 export const saveLedgerDerivationPath = (path: string) => {
   window?.localStorage.setItem(LS_KEY_DERIVATION_PATH, JSON.stringify(path));
@@ -38,19 +37,19 @@ export const isDeviceBusy = (transport: LedgerContextValue['transport']) => {
 export const getFirstIndexOnPage = (page: number, accountsPerPage: number) =>
   accountsPerPage * (page - 1);
 
+export const LedgerErrorsDict: Record<string, string | undefined> = {
+  TransportOpenUserCancelled: 'The connection attempt has been rejected.',
+  TransportStatusError:
+    'Make sure the device is connected and the Ethereum app is open on the device.',
+  InvalidStateError:
+    'Make sure the device is connected and the Ethereum app is open on the device.',
+  LockedDeviceError: 'The device is locked. Please, unlock it and try again.',
+  TransportError: undefined,
+};
+
 export const interceptLedgerError = (error: Error) => {
-  const errorDict: { [k: string]: string | undefined } = {
-    TransportOpenUserCancelled: 'The connection attempt has been rejected.',
-    TransportStatusError:
-      'Make sure the device is connected and the Ethereum app is open on the device.',
-    InvalidStateError:
-      'Make sure the device is connected and the Ethereum app is open on the device.',
-    LockedDeviceError: 'The device is locked. Please, unlock it and try again.',
-    TransportError: undefined,
-  };
-
-  if (error.name in errorDict) {
-    return new Error(errorDict[error.name]);
+  if (Object.hasOwn(LedgerErrorsDict, error.name)) {
+    return new Error(LedgerErrorsDict[error.name]);
   }
 
   return error;

diff --git a/packages/connect-wallet-modal/src/components/Ledger/hooks.ts b/packages/connect-wallet-modal/src/components/Ledger/hooks.ts
@@ -59,7 +59,7 @@ export const useLedgerAccounts = ({
       };
 
       // iterate over derivation paths for the given page
-      for await (const [index, path] of Object.entries(derivationPaths)) {
+      for (const [index, path] of Object.entries(derivationPaths)) {
         // Ledger may become busy during these iterations.
         // For example, if a user quickly changes pages, there will be several conflicting getAccountRecords requests,
         // each calling its own cycle of getAddress requests, each of them makes a ledger device busy for a short amount of time.
@@ -68,18 +68,13 @@ export const useLedgerAccounts = ({
         } else {
           // Device is busy, make a set of attempts with a timeout, waiting for the device to release
           const MAX_ATTEMPTS = 10;
-          const ATTEMPT_TIMEOUT = 400;
+          const ATTEMPT_TIMEOUT = 200;
           for (let attempts = 0; attempts < MAX_ATTEMPTS; attempts++) {
-            await new Promise<void>((resolve) => {
-              setTimeout(async () => {
-                if (!isDeviceBusy(transport)) {
-                  await getAndPushAccount(index, path);
-                  // no more attempts required, stop the cycle
-                  attempts = MAX_ATTEMPTS;
-                }
-                resolve();
-              }, ATTEMPT_TIMEOUT);
-            });
+            await new Promise(resolve => setTimeout(resolve, ATTEMPT_TIMEOUT));
+            if (!isDeviceBusy(transport)) {
+              await getAndPushAccount(index, path);
+              break;
+            }
           }
         }
       }