Skip to content

Commit

Permalink
[wallet-ext] re-factor LedgerSigner to handle lazily connecting to Le…
Browse files Browse the repository at this point in the history
…dger and do some other clean-up type work (MystenLabs#9635)

## Description 
This PR refactors the Ledger signing flow so that `useSigner` doesn't
return an `async` function. As I was prepping this for multi-Ledger
support, I also figured that it would be better to always prompt the
user to select what Ledger device they'd like to sign transactions with
instead of just connecting to the first connected device.

## Test Plan 
- Manual testing (signed/staked/approved transaction requests with and
without a Ledger account)

---
If your changes are not user-facing and not a breaking change, you can
skip the following section. Otherwise, please indicate what changed, and
then add to the Release Notes section as highlighted during the release
process.

### Type of Change (Check all that apply)

- [X] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes
N/A
  • Loading branch information
williamrobertson13 authored Mar 22, 2023
1 parent 117d4bc commit 0e5dfca
Show file tree
Hide file tree
Showing 11 changed files with 67 additions and 182 deletions.
26 changes: 19 additions & 7 deletions apps/wallet/src/ui/app/LedgerSigner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,37 +14,49 @@ import {
import type SuiLedgerClient from '@mysten/ledgerjs-hw-app-sui';

export class LedgerSigner extends SignerWithProvider {
readonly #suiLedgerClient: SuiLedgerClient;
#suiLedgerClient: SuiLedgerClient | null;
readonly #connectToLedger: () => Promise<SuiLedgerClient>;
readonly #derivationPath: string;
readonly #signatureScheme: SignatureScheme = 'ED25519';

constructor(
suiLedgerClient: SuiLedgerClient,
connectToLedger: () => Promise<SuiLedgerClient>,
derivationPath: string,
provider: JsonRpcProvider
) {
super(provider);
this.#suiLedgerClient = suiLedgerClient;
this.#connectToLedger = connectToLedger;
this.#suiLedgerClient = null;
this.#derivationPath = derivationPath;
}

async #initializeSuiLedgerClient() {
if (!this.#suiLedgerClient) {
this.#suiLedgerClient = await this.#connectToLedger();
}
return this.#suiLedgerClient;
}

async getAddress(): Promise<SuiAddress> {
const publicKeyResult = await this.#suiLedgerClient.getPublicKey(
const ledgerClient = await this.#initializeSuiLedgerClient();
const publicKeyResult = await ledgerClient.getPublicKey(
this.#derivationPath
);
const publicKey = new Ed25519PublicKey(publicKeyResult.publicKey);
return publicKey.toSuiAddress();
}

async getPublicKey(): Promise<Ed25519PublicKey> {
const { publicKey } = await this.#suiLedgerClient.getPublicKey(
const ledgerClient = await this.#initializeSuiLedgerClient();
const { publicKey } = await ledgerClient.getPublicKey(
this.#derivationPath
);
return new Ed25519PublicKey(publicKey);
}

async signData(data: Uint8Array): Promise<SerializedSignature> {
const { signature } = await this.#suiLedgerClient.signTransaction(
const ledgerClient = await this.#initializeSuiLedgerClient();
const { signature } = await ledgerClient.signTransaction(
this.#derivationPath,
data
);
Expand All @@ -58,7 +70,7 @@ export class LedgerSigner extends SignerWithProvider {

connect(provider: JsonRpcProvider): SignerWithProvider {
return new LedgerSigner(
this.#suiLedgerClient,
this.#connectToLedger,
this.#derivationPath,
provider
);
Expand Down
7 changes: 0 additions & 7 deletions apps/wallet/src/ui/app/components/ledger/LedgerExceptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,3 @@ export class LedgerNoTransportMechanismError extends Error {
Object.setPrototypeOf(this, LedgerNoTransportMechanismError.prototype);
}
}

export class LedgerDeviceNotFoundError extends Error {
constructor(message: string) {
super(message);
Object.setPrototypeOf(this, LedgerDeviceNotFoundError.prototype);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,8 @@ import {
useState,
} from 'react';

import { LedgerSigner } from '../../LedgerSigner';
import { api } from '../../redux/store/thunk-extras';
import {
LedgerConnectionFailedError,
LedgerDeviceNotFoundError,
LedgerNoTransportMechanismError,
} from './LedgerExceptions';

Expand All @@ -28,9 +25,6 @@ type SuiLedgerClientProviderProps = {
type SuiLedgerClientContextValue = {
suiLedgerClient: SuiLedgerClient | undefined;
connectToLedger: () => Promise<SuiLedgerClient>;
initializeLedgerSignerInstance: (
derivationPath: string
) => Promise<LedgerSigner>;
};

const SuiLedgerClientContext = createContext<
Expand All @@ -51,33 +45,6 @@ export function SuiLedgerClientProvider({
return () => suiLedgerClient?.transport.off('disconnect', onDisconnect);
}, [suiLedgerClient?.transport]);

const initializeLedgerSignerInstance = useCallback(
async (derivationPath: string) => {
if (!suiLedgerClient) {
try {
const transport = await forceConnectionToLedger();
const newClient = new SuiLedgerClient(transport);
setSuiLedgerClient(newClient);

return new LedgerSigner(
newClient,
derivationPath,
api.instance.fullNode
);
} catch (error) {
throw new Error('Failed to connect');
}
}

return new LedgerSigner(
suiLedgerClient,
derivationPath,
api.instance.fullNode
);
},
[suiLedgerClient]
);

const connectToLedger = useCallback(async () => {
if (suiLedgerClient?.transport) {
// If we've already connected to a Ledger device, we need
Expand All @@ -95,9 +62,8 @@ export function SuiLedgerClientProvider({
return {
suiLedgerClient,
connectToLedger,
initializeLedgerSignerInstance,
};
}, [connectToLedger, suiLedgerClient, initializeLedgerSignerInstance]);
}, [connectToLedger, suiLedgerClient]);

return (
<SuiLedgerClientContext.Provider value={contextValue}>
Expand Down Expand Up @@ -132,25 +98,3 @@ async function requestConnectionToLedger() {
"There are no supported transport mechanisms to connect to the user's Ledger device"
);
}

async function forceConnectionToLedger() {
let transport: TransportWebHID | TransportWebUSB | null | undefined;
try {
if (await TransportWebHID.isSupported()) {
transport = await TransportWebHID.openConnected();
} else if (await TransportWebUSB.isSupported()) {
transport = await TransportWebUSB.openConnected();
}
} catch (error) {
throw new LedgerConnectionFailedError(
"Unable to connect to the user's Ledger device"
);
}

if (!transport) {
throw new LedgerDeviceNotFoundError(
'Connected Ledger device not found'
);
}
return transport;
}
19 changes: 10 additions & 9 deletions apps/wallet/src/ui/app/hooks/useSigner.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

import { LedgerSigner } from '../LedgerSigner';
import { useSuiLedgerClient } from '../components/ledger/SuiLedgerClientProvider';
import { useAccounts } from './useAccounts';
import { useActiveAccount } from './useActiveAccount';
Expand All @@ -16,19 +17,19 @@ export function useSigner(address?: SuiAddress) {
? existingAccounts.find((account) => account.address === address)
: activeAccount;

const { initializeLedgerSignerInstance } = useSuiLedgerClient();
const { connectToLedger } = useSuiLedgerClient();
const { api, background } = thunkExtras;

if (!signerAccount) {
throw new Error("Can't find account for the signer address");
}

return async () => {
if (signerAccount.type === AccountType.LEDGER) {
return await initializeLedgerSignerInstance(
signerAccount.derivationPath
);
}
return api.getSignerInstance(signerAccount, background);
};
if (signerAccount.type === AccountType.LEDGER) {
return new LedgerSigner(
connectToLedger,
signerAccount.derivationPath,
api.instance.fullNode
);
}
return api.getSignerInstance(signerAccount, background);
}
6 changes: 2 additions & 4 deletions apps/wallet/src/ui/app/hooks/useTransactionDryRun.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,9 @@ export function useTransactionDryRun(
const signer = useSigner(sender);
const response = useQuery({
queryKey: ['dryRunTransaction', transaction.serialize()],
queryFn: async () => {
const initializedSigner = await signer();
return initializedSigner.dryRunTransaction({ transaction });
queryFn: () => {
return signer.dryRunTransaction({ transaction });
},
enabled: !!signer,
});
return response;
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,9 @@

import { fromB64 } from '@mysten/sui.js';
import { useMemo } from 'react';
import toast from 'react-hot-toast';

import { useSuiLedgerClient } from '../../components/ledger/SuiLedgerClientProvider';
import { UserApproveContainer } from '../../components/user-approve-container';
import { useAppDispatch } from '../../hooks';
import { useAccounts } from '../../hooks/useAccounts';
import { useAppDispatch, useSigner } from '../../hooks';
import { respondToTransactionRequest } from '../../redux/slices/transaction-requests';
import { Heading } from '../../shared/heading';
import { PageMainLayoutTitle } from '../../shared/page-main-layout/PageMainLayoutTitle';
Expand Down Expand Up @@ -38,12 +35,8 @@ export function SignMessageRequest({ request }: SignMessageRequestProps) {
};
}, [request.tx.message]);

const accounts = useAccounts();
const accountForTransaction = accounts.find(
(account) => account.address === request.tx.accountAddress
);
const signer = useSigner(request.tx.accountAddress);
const dispatch = useAppDispatch();
const { initializeLedgerSignerInstance } = useSuiLedgerClient();

return (
<UserApproveContainer
Expand All @@ -52,20 +45,13 @@ export function SignMessageRequest({ request }: SignMessageRequestProps) {
approveTitle="Sign"
rejectTitle="Reject"
onSubmit={(approved) => {
if (accountForTransaction) {
dispatch(
respondToTransactionRequest({
txRequestID: request.id,
approved,
accountForTransaction,
initializeLedgerSignerInstance,
})
);
} else {
toast.error(
`Account for address ${request.tx.accountAddress} not found`
);
}
dispatch(
respondToTransactionRequest({
txRequestID: request.id,
approved,
signer,
})
);
}}
address={request.tx.accountAddress}
scrollable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,13 @@
// import { Transaction } from '@mysten/sui.js';
import { Transaction } from '@mysten/sui.js';
import { useCallback, useMemo } from 'react';
import toast from 'react-hot-toast';

import { GasFees } from './GasFees';
import { TransactionDetails } from './TransactionDetails';
import { UserApproveContainer } from '_components/user-approve-container';
import { useAppDispatch } from '_hooks';
import { useAppDispatch, useSigner } from '_hooks';
import { type TransactionApprovalRequest } from '_payloads/transactions/ApprovalRequest';
import { respondToTransactionRequest } from '_redux/slices/transaction-requests';
import { useSuiLedgerClient } from '_src/ui/app/components/ledger/SuiLedgerClientProvider';
import { useAccounts } from '_src/ui/app/hooks/useAccounts';
import { PageMainLayoutTitle } from '_src/ui/app/shared/page-main-layout/PageMainLayoutTitle';

import st from './TransactionRequest.module.scss';
Expand All @@ -23,44 +20,28 @@ export type TransactionRequestProps = {
};

export function TransactionRequest({ txRequest }: TransactionRequestProps) {
const accounts = useAccounts();
const accountForTransaction = accounts.find(
(account) => account.address === txRequest.tx.account
);
const { initializeLedgerSignerInstance } = useSuiLedgerClient();
const addressForTransaction = txRequest.tx.account;
const signer = useSigner(addressForTransaction);
const dispatch = useAppDispatch();
const transaction = useMemo(() => {
const tx = Transaction.from(txRequest.tx.data);
if (accountForTransaction) {
tx.setSenderIfNotSet(accountForTransaction.address);
if (addressForTransaction) {
tx.setSenderIfNotSet(addressForTransaction);
}
return tx;
}, [txRequest.tx.data, accountForTransaction]);
const addressForTransaction = txRequest.tx.account;
}, [txRequest.tx.data, addressForTransaction]);

const handleOnSubmit = useCallback(
async (approved: boolean) => {
if (accountForTransaction) {
await dispatch(
respondToTransactionRequest({
approved,
txRequestID: txRequest.id,
accountForTransaction,
initializeLedgerSignerInstance,
})
);
} else {
toast.error(
`Account for address ${txRequest.tx.account} not found`
);
}
(approved: boolean) => {
dispatch(
respondToTransactionRequest({
approved,
txRequestID: txRequest.id,
signer,
})
);
},
[
accountForTransaction,
dispatch,
txRequest.id,
txRequest.tx.account,
initializeLedgerSignerInstance,
]
[dispatch, txRequest.id, signer]
);

return (
Expand All @@ -80,11 +61,11 @@ export function TransactionRequest({ txRequest }: TransactionRequestProps) {
address={addressForTransaction}
/> */}
<GasFees
sender={accountForTransaction?.address}
sender={addressForTransaction}
transaction={transaction}
/>
<TransactionDetails
sender={accountForTransaction?.address}
sender={addressForTransaction}
transaction={transaction}
/>
</section>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ export function TransferNFTForm({ objectId }: { objectId: string }) {
const tx = new Transaction();
tx.transferObjects([tx.object(objectId)], tx.pure(to));

const initializedSigner = await signer();
return initializedSigner.signAndExecuteTransaction({
return signer.signAndExecuteTransaction({
transaction: tx,
options: {
showInput: true,
Expand Down
5 changes: 2 additions & 3 deletions apps/wallet/src/ui/app/pages/home/transfer-coin/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import BottomMenuLayout, {
} from '_app/shared/bottom-menu-layout';
import { ActiveCoinsCard } from '_components/active-coins-card';
import Overlay from '_components/overlay';
import { useSigner } from '_hooks';
import { trackEvent } from '_src/shared/plausible';
import { useSigner } from '_src/ui/app/hooks';
import { useActiveAddress } from '_src/ui/app/hooks/useActiveAddress';

import type { SubmitProps } from './SendTokenForm';
Expand Down Expand Up @@ -62,8 +62,7 @@ function TransferCoinPage() {
props: { coinType: coinType! },
});

const initializedSigner = await signer();
return initializedSigner.signAndExecuteTransaction({
return signer.signAndExecuteTransaction({
transaction,
options: {
showInput: true,
Expand Down
Loading

0 comments on commit 0e5dfca

Please sign in to comment.