From e77ffe73ef020ab42915ef072399e2df434ac1b5 Mon Sep 17 00:00:00 2001 From: Alexander Khramov Date: Thu, 21 Sep 2023 14:46:46 +0300 Subject: [PATCH] fix: issues found during review --- apps/demo-react/pages/index.tsx | 8 +-- .../components/Ledger/LedgerAccountButton.tsx | 12 ++-- .../components/Ledger/LedgerAccountScreen.tsx | 67 +++++++++---------- .../src/components/Ledger/LedgerContext.tsx | 25 ++++--- .../Ledger/LedgerDerivationPathSelect.tsx | 2 +- .../src/components/Ledger/LedgerModal.tsx | 14 ++-- .../src/components/Ledger/helpers.ts | 29 ++++---- .../src/components/Ledger/hooks.ts | 19 ++---- 8 files changed, 81 insertions(+), 95 deletions(-) diff --git a/apps/demo-react/pages/index.tsx b/apps/demo-react/pages/index.tsx index 109ce4a7..3bb3b8e4 100644 --- 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 ( diff --git a/packages/connect-wallet-modal/src/components/Ledger/LedgerAccountButton.tsx b/packages/connect-wallet-modal/src/components/Ledger/LedgerAccountButton.tsx index 07bf5cf7..5d284b25 100644 --- 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 index 83742d3b..cf5a3e54 100644 --- 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 = ({ 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 = ({ metrics, closeScreen }) => { { - handleDerivationPathSelect(value); - }} + onChange={handleDerivationPathSelect} /> {/* 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 ( - { - void handleAccountButtonClick(account); - }} - /> - ); - }) - : Array.from({ length: ACCOUNTS_PER_PAGE }).map((_, 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 ( + { + void handleAccountButtonClick(account); + }} + /> + ); + } + : (_, i) => , + )} + = ({ 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 = ({ }, [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 index dcdfbf5e..e1e83c23 100644 --- 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 index 45e54c43..ab2c082b 100644 --- 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 ( - + @@ -42,16 +38,16 @@ export const LedgerScreen = ({ metrics, onClose }: LedgerModalProps) => { return ( - {error ? ( + {error && ( void reconnectTransport()} /> - ) : isTransportConnected ? ( + )} + {!error && isTransportConnected && ( - ) : ( - )} + {!error && !isTransportConnected && } ); }; diff --git a/packages/connect-wallet-modal/src/components/Ledger/helpers.ts b/packages/connect-wallet-modal/src/components/Ledger/helpers.ts index c9f83858..f56bf5bf 100644 --- 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 = { + 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 index 4da2968e..f1e618e5 100644 --- 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((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; + } } } }