Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Ledger - cancel properly and don't allow user to close non cancellable sections #5353

Merged
merged 5 commits into from
May 15, 2024
Merged

fix: Ledger - cancel properly and don't allow user to close non cancellable sections #5353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
89257f3
fix: go back properly and don't allow user to close non cancellable l…
pete-watters Apr 9, 2024
ffd79f4
fix(ledger): use correct key number
kyranjamie May 10, 2024
649a1ec
chore: refactor canclose
pete-watters May 14, 2024
772dea7
chore: add backgroundLocation for device busy step
pete-watters May 15, 2024
910a39d
fix: show correct UI count for BTC addresses being pulled, ref #4983
pete-watters May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/app/components/broadcast-error-dialog/broadcast-error-dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import get from 'lodash.get';
import { Button } from '@app/ui/components/button/button';
import { Dialog } from '@app/ui/components/containers/dialog/dialog';
import { Footer } from '@app/ui/components/containers/footers/footer';
import { Header } from '@app/ui/components/containers/headers/header';
import { DialogHeader } from '@app/ui/components/containers/headers/dialog-header';

export function BroadcastErrorDialog() {
const navigate = useNavigate();
Expand All @@ -17,7 +17,7 @@ export function BroadcastErrorDialog() {
return (
<Dialog
isShowing
header={<Header variant="dialog" />}
header={<DialogHeader />}
onClose={() => navigate('..')}
footer={
<Footer>
Expand Down
4 changes: 2 additions & 2 deletions src/app/features/dialogs/edit-nonce-dialog/edit-nonce-dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { StacksSendFormValues, StacksTransactionFormValues } from '@shared/model
import { useOnMount } from '@app/common/hooks/use-on-mount';
import { openInNewTab } from '@app/common/utils/open-in-new-tab';
import { Dialog } from '@app/ui/components/containers/dialog/dialog';
import { Header } from '@app/ui/components/containers/headers/header';
import { DialogHeader } from '@app/ui/components/containers/headers/dialog-header';
import { Link } from '@app/ui/components/link/link';

import { EditNonceForm } from './components/edit-nonce-form';
Expand Down Expand Up @@ -48,7 +48,7 @@ export function EditNonceDialog() {
}, [loadedNextNonce, onGoBack, setFieldError, setFieldValue, values.nonce]);

return (
<Dialog isShowing onClose={onClose} header={<Header variant="dialog" title="Edit nonce" />}>
<Dialog isShowing onClose={onClose} header={<DialogHeader title="Edit nonce" />}>
<Stack gap="space.05" pb="space.06" px="space.05">
<styled.span textStyle="caption.01">
If your transaction has been pending for a long time, its nonce might not be correct.
Expand Down
4 changes: 2 additions & 2 deletions src/app/features/dialogs/high-fee-dialog/high-fee-dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { openInNewTab } from '@app/common/utils/open-in-new-tab';
import { Button } from '@app/ui/components/button/button';
import { Dialog } from '@app/ui/components/containers/dialog/dialog';
import { Footer } from '@app/ui/components/containers/footers/footer';
import { Header } from '@app/ui/components/containers/headers/header';
import { DialogHeader } from '@app/ui/components/containers/headers/dialog-header';
import { Link } from '@app/ui/components/link/link';
import { Caption } from '@app/ui/components/typography/caption';
import { Title } from '@app/ui/components/typography/title';
Expand All @@ -38,7 +38,7 @@ export function HighFeeDialog({ learnMoreUrl, isShowing = false }: HighFeeDialog
>();
return (
<Dialog
header={<Header variant="dialog" />}
header={<DialogHeader />}
isShowing={isShowingHighFeeConfirmation}
onClose={() => setIsShowingHighFeeConfirmation(false)}
footer={
Expand Down
4 changes: 2 additions & 2 deletions src/app/features/dialogs/increase-fee-dialog/increase-btc-fee-dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { useBtcCryptoAssetBalanceNativeSegwit } from '@app/query/bitcoin/balance
import { useCurrentAccountNativeSegwitIndexZeroSigner } from '@app/store/accounts/blockchain/bitcoin/native-segwit-account.hooks';
import { Dialog } from '@app/ui/components/containers/dialog/dialog';
import { Footer } from '@app/ui/components/containers/footers/footer';
import { Header } from '@app/ui/components/containers/headers/header';
import { DialogHeader } from '@app/ui/components/containers/headers/dialog-header';
import { Spinner } from '@app/ui/components/spinner';
import { Caption } from '@app/ui/components/typography/caption';

Expand Down Expand Up @@ -69,7 +69,7 @@ export function IncreaseBtcFeeDialog() {
<Dialog
isShowing={location.pathname === RouteUrls.IncreaseBtcFee}
onClose={onClose}
header={<Header variant="dialog" title="Increase fee" />}
header={<DialogHeader title="Increase fee" />}
footer={
<Footer flexDirection="row">
<IncreaseFeeActions isDisabled={false} onCancel={() => navigate(RouteUrls.Home)} />
Expand Down
4 changes: 2 additions & 2 deletions src/app/features/dialogs/increase-fee-dialog/increase-fee-sent-dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { Flex } from 'leather-styles/jsx';
import { RouteUrls } from '@shared/route-urls';

import { Dialog } from '@app/ui/components/containers/dialog/dialog';
import { Header } from '@app/ui/components/containers/headers/header';
import { DialogHeader } from '@app/ui/components/containers/headers/dialog-header';
import { CheckmarkIcon } from '@app/ui/icons/checkmark-icon';

export function IncreaseFeeSentDialog() {
Expand All @@ -18,7 +18,7 @@ export function IncreaseFeeSentDialog() {
<Dialog
isShowing={isShowing}
onClose={() => navigate(RouteUrls.Home)}
header={<Header variant="dialog" title="Confirmed" />}
header={<DialogHeader title="Confirmed" />}
>
<Flex justifyContent="center" pb="space.06" px="space.05">
<CheckmarkIcon mt="2px" width="lg" />
Expand Down
4 changes: 2 additions & 2 deletions src/app/features/dialogs/increase-fee-dialog/increase-stx-fee-dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { useSubmittedTransactionsActions } from '@app/store/submitted-transactio
import { useRawDeserializedTxState, useRawTxIdState } from '@app/store/transactions/raw.hooks';
import { Dialog } from '@app/ui/components/containers/dialog/dialog';
import { Footer } from '@app/ui/components/containers/footers/footer';
import { Header } from '@app/ui/components/containers/headers/header';
import { DialogHeader } from '@app/ui/components/containers/headers/dialog-header';
import { Spinner } from '@app/ui/components/spinner';
import { Caption } from '@app/ui/components/typography/caption';

Expand Down Expand Up @@ -101,7 +101,7 @@ export function IncreaseStxFeeDialog() {
<Dialog
isShowing={location.pathname === RouteUrls.IncreaseStxFee}
onClose={onClose}
header={<Header variant="dialog" title="Increase fee" />}
header={<DialogHeader title="Increase fee" />}
footer={
<Footer flexDirection="row">
<IncreaseFeeActions
Expand Down
38 changes: 19 additions & 19 deletions src/app/features/dialogs/switch-account-dialog/components/account-list-unavailable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
import { memo } from 'react';

import { Box, Flex, styled } from 'leather-styles/jsx';

export const AccountListUnavailable = memo(() => (
<Flex
flexDirection="column"
justifyContent="center"
minHeight="120px"
mb="space.06"
px="space.05"
>
<Box>
<styled.span textStyle="label.01">Unable to load account information</styled.span>
<styled.span mt="space.03" textStyle="body.02">
We're unable to load information about your accounts. This may be a problem with the
wallet's API. If this problem persists, contact support.
</styled.span>
</Box>
</Flex>
));
export function AccountListUnavailable() {
return (
<Flex
flexDirection="column"
justifyContent="center"
minHeight="120px"
mb="space.06"
px="space.05"
>
<Box>
<styled.span textStyle="label.01">Unable to load account information</styled.span>
<styled.span mt="space.03" textStyle="body.02">
We're unable to load information about your accounts. This may be a problem with the
wallet's API. If this problem persists, contact support.
</styled.span>
</Box>
</Flex>
);
}
13 changes: 9 additions & 4 deletions src/app/features/dialogs/switch-account-dialog/switch-account-dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ import { useCurrentAccountIndex } from '@app/store/accounts/account';
import { useFilteredBitcoinAccounts } from '@app/store/accounts/blockchain/bitcoin/bitcoin.ledger';
import { useStacksAccounts } from '@app/store/accounts/blockchain/stacks/stacks-account.hooks';
import { Button } from '@app/ui/components/button/button';
import { Dialog, DialogProps } from '@app/ui/components/containers/dialog/dialog';
import { Dialog } from '@app/ui/components/containers/dialog/dialog';
import { Footer } from '@app/ui/components/containers/footers/footer';
import { Header } from '@app/ui/components/containers/headers/header';
import { DialogHeader } from '@app/ui/components/containers/headers/dialog-header';
import { VirtuosoWrapper } from '@app/ui/components/virtuoso';

import { AccountListUnavailable } from './components/account-list-unavailable';
Expand All @@ -22,7 +22,12 @@ export interface SwitchAccountOutletContext {
setIsShowingSwitchAccount(isShowing: boolean): void;
}

export const SwitchAccountDialog = memo(({ isShowing, onClose }: DialogProps) => {
interface SwitchAccountDialogProps {
isShowing: boolean;
onClose(): void;
}

export const SwitchAccountDialog = memo(({ isShowing, onClose }: SwitchAccountDialogProps) => {
const currentAccountIndex = useCurrentAccountIndex();
const createAccount = useCreateAccount();
const { whenWallet } = useWalletType();
Expand All @@ -48,7 +53,7 @@ export const SwitchAccountDialog = memo(({ isShowing, onClose }: DialogProps) =>

return (
<Dialog
header={<Header variant="dialog" title="Select account" />}
header={<DialogHeader title="Select account" />}
isShowing={isShowing}
onClose={onClose}
wrapChildren={false}
Expand Down
5 changes: 3 additions & 2 deletions src/app/features/ledger/flows/bitcoin-tx-signing/ledger-bitcoin-sign-tx-container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
getBitcoinAppVersion,
isBitcoinAppOpen,
} from '@app/features/ledger/utils/bitcoin-ledger-utils';
import { useCancelLedgerAction } from '@app/features/ledger/utils/generic-ledger-utils';
import { useToast } from '@app/features/toasts/use-toast';
import { useSignLedgerBitcoinTx } from '@app/store/accounts/blockchain/bitcoin/bitcoin.hooks';
import { useCurrentNetwork } from '@app/store/networks/networks.selectors';
Expand Down Expand Up @@ -111,12 +112,12 @@ function LedgerSignBitcoinTxContainer() {
latestDeviceResponse,
awaitingDeviceConnection,
};
const canCancelLedgerAction = useCancelLedgerAction(awaitingDeviceConnection);

return (
<TxSigningFlow
context={ledgerContextValue}
awaitingDeviceConnection={awaitingDeviceConnection}
closeAction={ledgerNavigate.cancelLedgerAction}
closeAction={canCancelLedgerAction ? ledgerNavigate.cancelLedgerAction : undefined}
/>
);
}
16 changes: 6 additions & 10 deletions src/app/features/ledger/flows/jwt-signing/ledger-sign-jwt-container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,17 @@ import { useDefaultRequestParams } from '@app/common/hooks/use-default-request-s
import { useKeyActions } from '@app/common/hooks/use-key-actions';
import { useScrollLock } from '@app/common/hooks/use-scroll-lock';
import { makeLedgerCompatibleUnsignedAuthResponsePayload } from '@app/common/unsafe-auth-response';
import { useCancelLedgerAction } from '@app/features/ledger/utils/generic-ledger-utils';
import {
getStacksAppVersion,
prepareLedgerDeviceStacksAppConnection,
useActionCancellableByUser,
} from '@app/features/ledger/utils/stacks-ledger-utils';
import {
useCurrentStacksAccount,
useStacksAccounts,
} from '@app/store/accounts/blockchain/stacks/stacks-account.hooks';
import { Dialog } from '@app/ui/components/containers/dialog/dialog';
import { Header } from '@app/ui/components/containers/headers/header';
import { DialogHeader } from '@app/ui/components/containers/headers/dialog-header';

import { useLedgerNavigate } from '../../hooks/use-ledger-navigate';
import { checkLockedDeviceError, useLedgerResponseState } from '../../utils/generic-ledger-utils';
Expand All @@ -47,7 +47,6 @@ export function LedgerSignJwtContainer() {
const getBitcoinAddressesLegacyFormat = useGetLegacyAuthBitcoinAddresses();

const keyActions = useKeyActions();
const canUserCancelAction = useActionCancellableByUser();
const { decodedAuthRequest, authRequest } = useOnboardingState();

const [accountIndex, setAccountIndex] = useState<null | number>(null);
Expand Down Expand Up @@ -178,17 +177,14 @@ export function LedgerSignJwtContainer() {
awaitingDeviceConnection,
};

const canCancelLedgerAction = useCancelLedgerAction(awaitingDeviceConnection);

return (
<LedgerJwtSigningProvider value={ledgerContextValue}>
<Dialog
isShowing
header={
<Header
variant="dialog"
isWaitingOnPerformedAction={awaitingDeviceConnection || canUserCancelAction}
/>
}
onClose={onCancelConnectLedger}
header={<DialogHeader />}
onClose={canCancelLedgerAction ? () => onCancelConnectLedger() : undefined}
>
<Outlet />
</Dialog>
Expand Down
23 changes: 15 additions & 8 deletions src/app/features/ledger/flows/request-bitcoin-keys/ledger-request-bitcoin-keys.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,26 +9,28 @@ import { pullBitcoinKeysFromLedgerDevice } from '@app/features/ledger/flows/requ
import { ledgerRequestKeysRoutes } from '@app/features/ledger/generic-flows/request-keys/ledger-request-keys-route-generator';
import { LedgerRequestKeysContext } from '@app/features/ledger/generic-flows/request-keys/ledger-request-keys.context';
import { RequestKeysFlow } from '@app/features/ledger/generic-flows/request-keys/request-keys-flow';
import { useRequestLedgerKeys } from '@app/features/ledger/generic-flows/request-keys/use-request-ledger-keys';
import {
defaultNumberOfKeysToPullFromLedgerDevice,
useRequestLedgerKeys,
} from '@app/features/ledger/generic-flows/request-keys/use-request-ledger-keys';
import { useLedgerNavigate } from '@app/features/ledger/hooks/use-ledger-navigate';
import {
connectLedgerBitcoinApp,
getBitcoinAppVersion,
isBitcoinAppOpen,
} from '@app/features/ledger/utils/bitcoin-ledger-utils';
import { useActionCancellableByUser } from '@app/features/ledger/utils/stacks-ledger-utils';
import { useCancelLedgerAction } from '@app/features/ledger/utils/generic-ledger-utils';
import { bitcoinKeysSlice } from '@app/store/ledger/bitcoin/bitcoin-key.slice';
import { useCurrentNetwork } from '@app/store/networks/networks.selectors';

function LedgerRequestBitcoinKeys() {
const navigate = useNavigate();
const dispatch = useDispatch();

const canUserCancelAction = useActionCancellableByUser();
const ledgerNavigate = useLedgerNavigate();
const network = useCurrentNetwork();

const chain = 'bitcoin' as const;
const chain = 'bitcoin';

const { requestKeys, latestDeviceResponse, awaitingDeviceConnection } =
useRequestLedgerKeys<BitcoinApp>({
Expand All @@ -43,13 +45,17 @@ function LedgerRequestBitcoinKeys() {
const { keys } = await pullBitcoinKeysFromLedgerDevice(app)({
network: bitcoinNetworkModeToCoreNetworkMode(network.chain.bitcoin.bitcoinNetwork),
onRequestKey(index) {
if (index <= 4) {
const keyGroupFinalIndex = defaultNumberOfKeysToPullFromLedgerDevice - 1;
const isNativeSegwitkey = index <= keyGroupFinalIndex;
if (isNativeSegwitkey) {
ledgerNavigate.toDeviceBusyStep(
`Requesting Bitcoin Native Segwit address (${index + 1}…5)`
`Requesting Bitcoin Native Segwit address (${index + 1}…${defaultNumberOfKeysToPullFromLedgerDevice})`
pete-watters marked this conversation as resolved.
Show resolved Hide resolved
);
return;
}
ledgerNavigate.toDeviceBusyStep(`Requesting Bitcoin Taproot address (${index - 4}…5)`);
ledgerNavigate.toDeviceBusyStep(
`Requesting Bitcoin Taproot address (${index - keyGroupFinalIndex}…${defaultNumberOfKeysToPullFromLedgerDevice})`
);
},
});
dispatch(bitcoinKeysSlice.actions.addKeys(keys));
Expand All @@ -64,10 +70,11 @@ function LedgerRequestBitcoinKeys() {
outdatedAppVersionWarning: false,
};

const canCancelLedgerAction = useCancelLedgerAction(awaitingDeviceConnection);
return (
<RequestKeysFlow
context={ledgerContextValue}
isActionCancellableByUser={awaitingDeviceConnection || canUserCancelAction}
isActionCancellableByUser={canCancelLedgerAction}
/>
);
}
Expand Down
3 changes: 2 additions & 1 deletion src/app/features/ledger/flows/request-bitcoin-keys/request-bitcoin-keys.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,13 @@ export function pullBitcoinKeysFromLedgerDevice(bitcoinApp: BitcoinApp, targetId
});
keys.push({ id: createWalletIdDecoratedPath(path, 'default'), path, policy, targetId });
}

for (
let accountIndex = 0;
accountIndex < defaultNumberOfKeysToPullFromLedgerDevice;
accountIndex++
) {
Comment on lines +75 to 80
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure consistent handling of defaultNumberOfKeysToPullFromLedgerDevice.

The loop structure is correct, but consider extracting the repeated logic into a separate function to adhere to the DRY principle.

- for (
-   let accountIndex = 0;
-   accountIndex < defaultNumberOfKeysToPullFromLedgerDevice;
-   accountIndex++
- ) {
-   onRequestKey?.(accountIndex);
-   const { path, policy } = await getNativeSegwitExtendedPublicKey({
-     bitcoinApp,
-     fingerprint,
-     network,
-     accountIndex,
-   });
-   keys.push({ id: createWalletIdDecoratedPath(path, 'default'), path, policy, targetId });
- }

+ async function addKeys(getExtendedPublicKey, startIndex = 0) {
+   for (
+     let accountIndex = 0;
+     accountIndex < defaultNumberOfKeysToPullFromLedgerDevice;
+     accountIndex++
+   ) {
+     onRequestKey?.(accountIndex + startIndex);
+     const { path, policy } = await getExtendedPublicKey({
+       bitcoinApp,
+       fingerprint,
+       network,
+       accountIndex,
+     });
+     keys.push({ id: createWalletIdDecoratedPath(path, 'default'), path, policy, targetId });
+   }
+ }
+ await addKeys(getNativeSegwitExtendedPublicKey);
+ await addKeys(getTaprootExtendedPublicKey, defaultNumberOfKeysToPullFromLedgerDevice);
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
for (
let accountIndex = 0;
accountIndex < defaultNumberOfKeysToPullFromLedgerDevice;
accountIndex++
) {
async function addKeys(getExtendedPublicKey, startIndex = 0) {
for (
let accountIndex = 0;
accountIndex < defaultNumberOfKeysToPullFromLedgerDevice;
accountIndex++
) {
onRequestKey?.(accountIndex + startIndex);
const { path, policy } = await getExtendedPublicKey({
bitcoinApp,
fingerprint,
network,
accountIndex,
});
keys.push({ id: createWalletIdDecoratedPath(path, 'default'), path, policy, targetId });
}
}
await addKeys(getNativeSegwitExtendedPublicKey);
await addKeys(getTaprootExtendedPublicKey, defaultNumberOfKeysToPullFromLedgerDevice);

onRequestKey?.(accountIndex + 5);
onRequestKey?.(accountIndex + defaultNumberOfKeysToPullFromLedgerDevice);
const { path, policy } = await getTaprootExtendedPublicKey({
bitcoinApp,
fingerprint,
Expand Down
6 changes: 3 additions & 3 deletions src/app/features/ledger/flows/request-stacks-keys/ledger-request-stacks-keys.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ import {
useRequestLedgerKeys,
} from '@app/features/ledger/generic-flows/request-keys/use-request-ledger-keys';
import { useLedgerNavigate } from '@app/features/ledger/hooks/use-ledger-navigate';
import { useCancelLedgerAction } from '@app/features/ledger/utils/generic-ledger-utils';
import {
connectLedgerStacksApp,
getStacksAppVersion,
isStacksAppOpen,
useActionCancellableByUser,
} from '@app/features/ledger/utils/stacks-ledger-utils';
import { useToast } from '@app/features/toasts/use-toast';
import { stacksKeysSlice } from '@app/store/ledger/stacks/stacks-key.slice';
Expand All @@ -27,7 +27,6 @@ function LedgerRequestStacksKeys() {
const toast = useToast();
const navigate = useNavigate();
const ledgerNavigate = useLedgerNavigate();
const canUserCancelAction = useActionCancellableByUser();

const dispatch = useDispatch();

Expand Down Expand Up @@ -76,10 +75,11 @@ function LedgerRequestStacksKeys() {
outdatedAppVersionWarning,
};

const canCancelLedgerAction = useCancelLedgerAction(awaitingDeviceConnection);
return (
<RequestKeysFlow
context={ledgerContextValue}
isActionCancellableByUser={awaitingDeviceConnection || canUserCancelAction}
isActionCancellableByUser={canCancelLedgerAction}
/>
);
}
Expand Down
Loading
Loading