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.

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

feat: disable submit button for masp tx when using disconnected ledger device #1572

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 11 additions & 2 deletions apps/namadillo/src/App/Masp/MaspShield.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@ import {
import { allDefaultAccountsAtom } from "atoms/accounts";
import { namadaTransparentAssetsAtom } from "atoms/balance/atoms";
import { chainParametersAtom } from "atoms/chain/atoms";
import { ledgerStatusDataAtom } from "atoms/ledger";
import { rpcUrlAtom } from "atoms/settings";
import BigNumber from "bignumber.js";
import { useTransactionActions } from "hooks/useTransactionActions";
import { useTransfer } from "hooks/useTransfer";
import { wallets } from "integrations";
import invariant from "invariant";
import { useAtomValue } from "jotai";
import { useAtom, useAtomValue } from "jotai";
import { createTransferDataFromNamada } from "lib/transactions";
import { useState } from "react";
import { generatePath, useNavigate, useSearchParams } from "react-router-dom";
Expand All @@ -32,13 +33,17 @@ export const MaspShield: React.FC = () => {
const rpcUrl = useAtomValue(rpcUrlAtom);
const chainParameters = useAtomValue(chainParametersAtom);
const defaultAccounts = useAtomValue(allDefaultAccountsAtom);

const [ledgerStatus, setLedgerStatusStop] = useAtom(ledgerStatusDataAtom);
const { data: availableAssets, isLoading: isLoadingAssets } = useAtomValue(
namadaTransparentAssetsAtom
);

const { storeTransaction } = useTransactionActions();

const ledgerAccountInfo = ledgerStatus && {
deviceConnected: ledgerStatus.connected,
errorMessage: ledgerStatus.errorMessage,
};
const chainId = chainParameters.data?.chainId;
const sourceAddress = defaultAccounts.data?.find(
(account) => account.type !== AccountType.ShieldedKeys
Expand Down Expand Up @@ -120,6 +125,9 @@ export const MaspShield: React.FC = () => {
}
};

// We stop the ledger status check when the transfer is in progress
setLedgerStatusStop(isPerformingTransfer);

return (
<Panel className="relative min-h-[600px]">
<header className="flex flex-col items-center text-center mb-3 gap-6">
Expand All @@ -143,6 +151,7 @@ export const MaspShield: React.FC = () => {
onChangeSelectedAsset,
amount: displayAmount,
onChangeAmount: setDisplayAmount,
ledgerAccountInfo,
}}
destination={{
chain: namadaChain as Chain,
Expand Down
12 changes: 10 additions & 2 deletions apps/namadillo/src/App/Masp/MaspUnshield.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@ import {
import { allDefaultAccountsAtom } from "atoms/accounts";
import { namadaShieldedAssetsAtom } from "atoms/balance/atoms";
import { chainParametersAtom } from "atoms/chain/atoms";
import { ledgerStatusDataAtom } from "atoms/ledger/atoms";
import { rpcUrlAtom } from "atoms/settings";
import BigNumber from "bignumber.js";
import { useTransactionActions } from "hooks/useTransactionActions";
import { useTransfer } from "hooks/useTransfer";
import { wallets } from "integrations";
import invariant from "invariant";
import { useAtomValue } from "jotai";
import { useAtom, useAtomValue } from "jotai";
import { createTransferDataFromNamada } from "lib/transactions";
import { useState } from "react";
import { generatePath, useNavigate, useSearchParams } from "react-router-dom";
Expand All @@ -32,13 +33,17 @@ export const MaspUnshield: React.FC = () => {
const rpcUrl = useAtomValue(rpcUrlAtom);
const chainParameters = useAtomValue(chainParametersAtom);
const defaultAccounts = useAtomValue(allDefaultAccountsAtom);

const [ledgerStatus, setLedgerStatusStop] = useAtom(ledgerStatusDataAtom);
const { data: availableAssets, isLoading: isLoadingAssets } = useAtomValue(
namadaShieldedAssetsAtom
);

const { storeTransaction } = useTransactionActions();

const ledgerAccountInfo = ledgerStatus && {
deviceConnected: ledgerStatus.connected,
errorMessage: ledgerStatus.errorMessage,
};
const chainId = chainParameters.data?.chainId;
const account = defaultAccounts.data?.find(
(account) => account.type === AccountType.ShieldedKeys
Expand Down Expand Up @@ -120,6 +125,8 @@ export const MaspUnshield: React.FC = () => {
setGeneralErrorMessage(err + "");
}
};
// We stop the ledger status check when the transfer is in progress
setLedgerStatusStop(isPerformingTransfer);

return (
<Panel className="relative min-h-[600px]">
Expand All @@ -145,6 +152,7 @@ export const MaspUnshield: React.FC = () => {
onChangeSelectedAsset,
amount: displayAmount,
onChangeAmount: setDisplayAmount,
ledgerAccountInfo,
}}
destination={{
chain: namadaChain as Chain,
Expand Down
13 changes: 12 additions & 1 deletion apps/namadillo/src/App/NamadaTransfer/NamadaTransfer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@ import {
namadaTransparentAssetsAtom,
} from "atoms/balance/atoms";
import { chainParametersAtom } from "atoms/chain/atoms";
import { ledgerStatusDataAtom } from "atoms/ledger";
import { applicationFeaturesAtom, rpcUrlAtom } from "atoms/settings";
import BigNumber from "bignumber.js";
import { useTransactionActions } from "hooks/useTransactionActions";
import { useTransfer } from "hooks/useTransfer";
import { wallets } from "integrations";
import invariant from "invariant";
import { useAtomValue } from "jotai";
import { useAtom, useAtomValue } from "jotai";
import { createTransferDataFromNamada } from "lib/transactions";
import { useMemo, useState } from "react";
import { generatePath, useNavigate, useSearchParams } from "react-router-dom";
Expand All @@ -41,6 +42,7 @@ export const NamadaTransfer: React.FC = () => {
const features = useAtomValue(applicationFeaturesAtom);
const chainParameters = useAtomValue(chainParametersAtom);
const defaultAccounts = useAtomValue(allDefaultAccountsAtom);
const [ledgerStatus, setLedgerStatusStop] = useAtom(ledgerStatusDataAtom);

const { data: availableAssetsData, isLoading: isLoadingAssets } =
useAtomValue(
Expand All @@ -49,6 +51,10 @@ export const NamadaTransfer: React.FC = () => {

const { storeTransaction } = useTransactionActions();

const ledgerAccountInfo = ledgerStatus && {
deviceConnected: ledgerStatus.connected,
errorMessage: ledgerStatus.errorMessage,
};
const availableAssets = useMemo(() => {
if (features.namTransfersEnabled) {
return availableAssetsData;
Expand Down Expand Up @@ -153,6 +159,9 @@ export const NamadaTransfer: React.FC = () => {
}
};

// We stop the ledger status check when the transfer is in progress
setLedgerStatusStop(isPerformingTransfer);

return (
<Panel className="relative min-h-[600px]">
<header className="flex flex-col items-center text-center mb-3 gap-6">
Expand Down Expand Up @@ -181,12 +190,14 @@ export const NamadaTransfer: React.FC = () => {
onChangeShielded: setShielded,
amount: displayAmount,
onChangeAmount: setDisplayAmount,
ledgerAccountInfo,
}}
destination={{
chain: namadaChain as Chain,
enableCustomAddress: true,
customAddress,
onChangeCustomAddress: setCustomAddress,
isShielded: isTargetShielded,
}}
gasConfig={gasConfig}
isSubmitting={isPerformingTransfer}
Expand Down
14 changes: 14 additions & 0 deletions apps/namadillo/src/App/Transfer/TransferModule.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
Address,
AddressWithAssetAndAmountMap,
GasConfig,
LedgerAccountInfo,
WalletProvider,
} from "types";
import { getDisplayGasFee } from "utils/gas";
Expand All @@ -31,6 +32,8 @@ type TransferModuleConfig = {
onChangeChain?: (chain: Chain) => void;
isShielded?: boolean;
onChangeShielded?: (isShielded: boolean) => void;
// Additional information if selected account is a ledger
ledgerAccountInfo?: LedgerAccountInfo;
};

export type TransferSourceProps = TransferModuleConfig & {
Expand Down Expand Up @@ -87,6 +90,7 @@ type ValidationResult =
| "NoDestinationChain"
| "NoTransactionFee"
| "NotEnoughBalance"
| "NoLedgerConnected"
| "Ok";

export const TransferModule = ({
Expand Down Expand Up @@ -155,6 +159,12 @@ export const TransferModule = ({
return "NotEnoughBalance";
} else if (!destination.wallet && !destination.customAddress) {
return "NoDestinationWallet";
} else if (
(source.isShielded || destination.isShielded) &&
source.ledgerAccountInfo &&
!source.ledgerAccountInfo.deviceConnected
) {
return "NoLedgerConnected";
} else {
return "Ok";
}
Expand Down Expand Up @@ -236,6 +246,10 @@ export const TransferModule = ({
return "Select Asset";
}

if (validationResult === "NoLedgerConnected") {
return "Connect your ledger and open the Namada App";
}

// TODO: this should be updated for nfts
if (validationResult === "NoAmount") {
return "Define an amount to transfer";
Expand Down
8 changes: 8 additions & 0 deletions apps/namadillo/src/atoms/accounts/atoms.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { namadaExtensionConnectedAtom } from "atoms/settings";
import { queryDependentFn } from "atoms/utils";
import BigNumber from "bignumber.js";
import { NamadaKeychain } from "hooks/useNamadaKeychain";
import { atom } from "jotai";
import { atomWithMutation, atomWithQuery } from "jotai-tanstack-query";
import {
fetchAccountBalance,
Expand Down Expand Up @@ -69,6 +70,13 @@ export const allDefaultAccountsAtom = atomWithQuery<Account[]>((get) => {
};
});

export const isLedgerAccountAtom = atom((get) => {
const defaultAccounts = get(allDefaultAccountsAtom);
return Boolean(
defaultAccounts.data?.find((account) => account.type === AccountType.Ledger)
);
});

export const updateDefaultAccountAtom = atomWithMutation(() => {
const namadaPromise = new NamadaKeychain().get();
return {
Expand Down
75 changes: 75 additions & 0 deletions apps/namadillo/src/atoms/ledger/atoms.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import { ledgerUSBList } from "@namada/sdk/web";
import { LedgerError } from "@zondax/ledger-namada";
import { isLedgerAccountAtom } from "atoms/accounts";
import { atom } from "jotai";

import { atomWithQuery } from "jotai-tanstack-query";
import { getSdkInstance } from "utils/sdk";

export type LedgerStatus = {
connected: boolean;
errorMessage: string;
};

const ledgerStatusStopAtom = atom(false);

export const ledgerStatusAtom = atomWithQuery<LedgerStatus | undefined>(() => {
return {
refetchInterval: 1000,
queryKey: ["ledger-status"],
queryFn: async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think something may need to change here after testing. What I found was that this loop continues to execute after I've connected and submitted a Tx, and I hit a few cases with different errors thrown. It will definitely be an issue if we try to open transport on Web when the Keychain is attempting to open transport to sign. I'll give it some thought! Maybe if we, say, detect it once, if it's undetected, require the user to click to Connect, once connected, fetch build params then submit. I think maybe it would be better if validation happens once, or, if the check is discontinued after first successful connection? Not sure, will take some thought

const devices = await ledgerUSBList();

if (devices.length > 0) {
try {
// Disable console.warn to prevent the warning from showing up in the UI in the loop
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(console as any).warnOld = console.warn;
console.warn = () => {};

const sdk = await getSdkInstance();
const ledger = await sdk.initLedger();
const {
version: { returnCode, errorMessage },
} = await ledger.status();

const connected = returnCode === LedgerError.NoErrors;

await ledger.closeTransport();

return {
connected,
errorMessage,
};
} catch (e) {
return {
connected: false,
errorMessage: `${e}`,
};
} finally {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
console.warn = (console as any).warnOld;
}
}

return {
connected: false,
errorMessage: "Ledger device not detected",
};
},
};
});

export const ledgerStatusDataAtom = atom(
(get) => {
const isLedgerAccount = get(isLedgerAccountAtom);
const ledgetStatusStop = get(ledgerStatusStopAtom);

if (isLedgerAccount && !ledgetStatusStop) {
return get(ledgerStatusAtom).data;
}
},
(_, set, stop: boolean) => {
set(ledgerStatusStopAtom, stop);
}
);
1 change: 1 addition & 0 deletions apps/namadillo/src/atoms/ledger/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./atoms";
5 changes: 5 additions & 0 deletions apps/namadillo/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -376,3 +376,8 @@ export type LocalnetToml = {
chain_1_channel: string;
chain_2_channel: string;
};

export type LedgerAccountInfo = {
deviceConnected: boolean;
errorMessage: string;
};
2 changes: 1 addition & 1 deletion packages/sdk/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Make Ledger available for direct-import as it is not dependent on Sdk initialization
export { Ledger, initLedgerUSBTransport } from "./ledger";
export { Ledger, initLedgerUSBTransport, ledgerUSBList } from "./ledger";
export type {
LedgerAddressAndPublicKey,
LedgerProofGenerationKey,
Expand Down
8 changes: 8 additions & 0 deletions packages/sdk/src/ledger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,14 @@ export const initLedgerUSBTransport = async (): Promise<Transport> => {
return await TransportUSB.create();
};

/**
* Returns a list of ledger devices
* @async
* @returns List of USB devices
*/
export const ledgerUSBList = async (): Promise<USBDevice[]> => {
return await TransportUSB.list();
};
export const DEFAULT_LEDGER_BIP44_PATH = makeBip44Path(coinType, {
account: 0,
change: 0,
Expand Down