Skip to content

Commit

Permalink
[wallet-ext] import and save Ledger accounts to the wallet (#9165)
Browse files Browse the repository at this point in the history
## Description 

This PR splits the `Account` class into separate classes for each
account type: `LedgerAccount`, `ImportedAccount`, and `DerivedAccount`.
I think there are two options to go about supporting signing
transactions with Ledger in the wallet:
1. Make a keypair with async resolution that can connect to the user's
Ledger device from the background script
2. Remove the concept of a keypair for Ledger accounts and sign
transactions using a Ledger signer in the content script

While I think option 1 is possibly the more "proper" solution based on
some brief discussions with @Jordan-Mysten, there's added complexity to
updating all usages of the `Keypair` interface in the SDK (or creating
some discriminated union where `type Keypair =
KeypairWithAsyncResolution | KeypairWIthSyncResolution`). There's also
the matter of initializing a `SuiLedgerClient` instance from the
background script which has added complexity (how do we handle errors
when the user disconnects their device and then tries to sign a
transaction?).

So this PR implements the initial work for option 2 which is to remove
the concept of keypairs for Ledger accounts. While this might not be the
most ideal, I think it makes sense, and having distinct Account classes
allows us to be very explicit in code with what is or isn't possible for
an account (e.g., can I export the private key to this account?). If
option 1 might be more feasible or if there are concerns with this
approach, I'm very glad to hear any feedback 😄.

Part 1 (this diff): #9165
Part 2 (add the ability to save/load Ledger accounts):
#9166
Part 3 (hook-up UI to save imported Ledger accounts):
#9167
Part 4 (add account badge for Ledger accounts):
#9168

## Test Plan 
- Manual testing (No visible user changes and everything works the same
as before)

---
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)

- [ ] 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 12, 2023
1 parent 05573b2 commit d4d234f
Show file tree
Hide file tree
Showing 22 changed files with 499 additions and 146 deletions.
1 change: 1 addition & 0 deletions apps/core/tailwind.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ module.exports = {
subtitleSmallExtra: ['10px', '1'],
caption: ['12px', '1'],
captionSmall: ['11px', '1'],
captionSmallExtra: ['10px', '1'],
iconTextLarge: ['48px', '1'],

// Heading sizes:
Expand Down
98 changes: 43 additions & 55 deletions apps/wallet/src/background/keyring/Account.ts
Original file line number Diff line number Diff line change
@@ -1,66 +1,54 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

import {
normalizeSuiAddress,
toSerializedSignature,
type SerializedSignature,
type Keypair,
type SuiAddress,
} from '@mysten/sui.js';
import { type SuiAddress } from '@mysten/sui.js';

export type AccountType = 'derived' | 'imported';
export type AccountSerialized = {
type: AccountType;
address: SuiAddress;
derivationPath: string | null;
};
import {
type DerivedAccount,
type SerializedDerivedAccount,
} from './DerivedAccount';
import {
type ImportedAccount,
type SerializedImportedAccount,
} from './ImportedAccount';
import {
type LedgerAccount,
type SerializedLedgerAccount,
} from './LedgerAccount';

export enum AccountType {
IMPORTED = 'IMPORTED',
DERIVED = 'DERIVED',
LEDGER = 'LEDGER',
}

export class Account {
#keypair: Keypair;
public readonly type: AccountType;
public readonly derivationPath: string | null;
public readonly address: SuiAddress;
export type SerializedAccount =
| SerializedImportedAccount
| SerializedDerivedAccount
| SerializedLedgerAccount;

constructor(
options:
| { type: 'derived'; derivationPath: string; keypair: Keypair }
| { type: 'imported'; keypair: Keypair }
) {
this.type = options.type;
this.derivationPath =
options.type === 'derived' ? options.derivationPath : null;
this.#keypair = options.keypair;
this.address = normalizeSuiAddress(
this.#keypair.getPublicKey().toSuiAddress()
);
}
export interface Account {
readonly type: AccountType;
readonly address: SuiAddress;
toJSON(): SerializedAccount;
}

exportKeypair() {
return this.#keypair.export();
}
export function isImportedOrDerivedAccount(
account: Account
): account is ImportedAccount | DerivedAccount {
return isImportedAccount(account) || isDerivedAccount(account);
}

async sign(data: Uint8Array): Promise<SerializedSignature> {
const pubkey = this.#keypair.getPublicKey();
// This is fine to hardcode useRecoverable = false because wallet does not support Secp256k1. Ed25519 does not use this parameter.
const signature = this.#keypair.signData(data, false);
const signatureScheme = this.#keypair.getKeyScheme();
return toSerializedSignature({
signature,
signatureScheme,
pubKey: pubkey,
});
}
export function isImportedAccount(
account: Account
): account is ImportedAccount {
return account.type === AccountType.IMPORTED;
}

toJSON(): AccountSerialized {
return {
type: this.type,
address: this.address,
derivationPath: this.derivationPath,
};
}
export function isDerivedAccount(account: Account): account is DerivedAccount {
return account.type === AccountType.DERIVED;
}

get publicKey() {
return this.#keypair.getPublicKey();
}
export function isLedgerAccount(account: Account): account is LedgerAccount {
return account.type === AccountType.LEDGER;
}
36 changes: 36 additions & 0 deletions apps/wallet/src/background/keyring/AccountKeypair.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

import {
toSerializedSignature,
type SerializedSignature,
type Keypair,
} from '@mysten/sui.js';

export class AccountKeypair {
#keypair: Keypair;

constructor(keypair: Keypair) {
this.#keypair = keypair;
}

async sign(data: Uint8Array): Promise<SerializedSignature> {
const pubkey = this.#keypair.getPublicKey();
// This is fine to hardcode useRecoverable = false because wallet does not support Secp256k1. Ed25519 does not use this parameter.
const signature = this.#keypair.signData(data, false);
const signatureScheme = this.#keypair.getKeyScheme();
return toSerializedSignature({
signature,
signatureScheme,
pubKey: pubkey,
});
}

exportKeypair() {
return this.#keypair.export();
}

get publicKey() {
return this.#keypair.getPublicKey();
}
}
47 changes: 47 additions & 0 deletions apps/wallet/src/background/keyring/DerivedAccount.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

import {
normalizeSuiAddress,
type Keypair,
type SuiAddress,
} from '@mysten/sui.js';

import { type Account, AccountType } from './Account';
import { AccountKeypair } from './AccountKeypair';

export type SerializedDerivedAccount = {
type: AccountType.DERIVED;
address: SuiAddress;
derivationPath: string;
};

export class DerivedAccount implements Account {
readonly accountKeypair: AccountKeypair;
readonly type: AccountType;
readonly address: SuiAddress;
readonly derivationPath: string;

constructor({
derivationPath,
keypair,
}: {
derivationPath: string;
keypair: Keypair;
}) {
this.type = AccountType.IMPORTED;
this.derivationPath = derivationPath;
this.accountKeypair = new AccountKeypair(keypair);
this.address = normalizeSuiAddress(
this.accountKeypair.publicKey.toSuiAddress()
);
}

toJSON(): SerializedDerivedAccount {
return {
type: AccountType.DERIVED,
address: this.address,
derivationPath: this.derivationPath,
};
}
}
39 changes: 39 additions & 0 deletions apps/wallet/src/background/keyring/ImportedAccount.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

import {
normalizeSuiAddress,
type Keypair,
type SuiAddress,
} from '@mysten/sui.js';

import { type Account, AccountType } from './Account';
import { AccountKeypair } from './AccountKeypair';

export type SerializedImportedAccount = {
type: AccountType.IMPORTED;
address: SuiAddress;
derivationPath: null;
};

export class ImportedAccount implements Account {
readonly accountKeypair: AccountKeypair;
readonly type: AccountType;
readonly address: SuiAddress;

constructor({ keypair }: { keypair: Keypair }) {
this.type = AccountType.IMPORTED;
this.accountKeypair = new AccountKeypair(keypair);
this.address = normalizeSuiAddress(
this.accountKeypair.publicKey.toSuiAddress()
);
}

toJSON(): SerializedImportedAccount {
return {
type: AccountType.IMPORTED,
address: this.address,
derivationPath: null,
};
}
}
38 changes: 38 additions & 0 deletions apps/wallet/src/background/keyring/LedgerAccount.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

import { normalizeSuiAddress, type SuiAddress } from '@mysten/sui.js';

import { type Account, AccountType } from './Account';

export type SerializedLedgerAccount = {
type: AccountType.LEDGER;
address: SuiAddress;
derivationPath: string;
};

export class LedgerAccount implements Account {
readonly type: AccountType;
readonly address: SuiAddress;
readonly derivationPath: string;

constructor({
address,
derivationPath,
}: {
address: SuiAddress;
derivationPath: string;
}) {
this.type = AccountType.LEDGER;
this.address = normalizeSuiAddress(address);
this.derivationPath = derivationPath;
}

toJSON(): SerializedLedgerAccount {
return {
type: AccountType.LEDGER,
address: this.address,
derivationPath: this.derivationPath,
};
}
}
8 changes: 6 additions & 2 deletions apps/wallet/src/background/keyring/VaultStorage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
setToSessionStorage,
isSessionStorageSupported,
} from '../storage-utils';
import { Account } from './Account';
import { ImportedAccount } from './ImportedAccount';
import {
EPHEMERAL_PASSWORD_KEY,
EPHEMERAL_VAULT_KEY,
Expand Down Expand Up @@ -217,7 +217,11 @@ describe('VaultStorage', () => {
await VaultStorage.importKeypair(
testEd25519Serialized,
testDataVault1.password,
[new Account({ type: 'imported', keypair: testEd25519 })]
[
new ImportedAccount({
keypair: testEd25519,
}),
]
)
).toBe(null);
});
Expand Down
14 changes: 8 additions & 6 deletions apps/wallet/src/background/keyring/VaultStorage.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

import { fromExportedKeypair } from '@mysten/sui.js';
import { fromExportedKeypair, normalizeSuiAddress } from '@mysten/sui.js';
import { randomBytes } from '@noble/hashes/utils';

import {
Expand All @@ -11,7 +11,8 @@ import {
setToLocalStorage,
setToSessionStorage,
} from '../storage-utils';
import { type Account } from './Account';
import { type DerivedAccount } from './DerivedAccount';
import { type ImportedAccount } from './ImportedAccount';
import { Vault } from './Vault';
import { getRandomEntropy, toEntropy } from '_shared/utils/bip39';

Expand Down Expand Up @@ -139,16 +140,17 @@ class VaultStorageClass {
public async importKeypair(
keypair: ExportedKeypair,
password: string,
existingAccounts: Account[]
existingAccounts: (ImportedAccount | DerivedAccount)[]
) {
if (!this.#vault) {
throw new Error('Error, vault is locked. Unlock the vault first.');
}
const keypairToImport = fromExportedKeypair(keypair);
const importedAddress = keypairToImport.getPublicKey().toSuiAddress();
const importedAddress = normalizeSuiAddress(
keypairToImport.getPublicKey().toSuiAddress()
);
const isDuplicate = existingAccounts.some(
(anAccount) =>
anAccount.publicKey.toSuiAddress() === importedAddress
(anAccount) => anAccount.address === importedAddress
);
if (isDuplicate) {
return null;
Expand Down
8 changes: 4 additions & 4 deletions apps/wallet/src/background/keyring/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { describe, expect, it, vi, beforeEach } from 'vitest';

import { Keyring } from '.';
import { getFromLocalStorage, setToLocalStorage } from '../storage-utils';
import { type DerivedAccount } from './DerivedAccount';
import { VaultStorage } from './VaultStorage';
import Alarm from '_src/background/Alarms';
import {
Expand Down Expand Up @@ -59,12 +60,11 @@ describe('Keyring', () => {

describe('getActiveAccount', () => {
it('returns as active account the first derived from mnemonic', async () => {
expect((await k.getActiveAccount())!.address).toBe(
const account = (await k.getActiveAccount()) as DerivedAccount;
expect(account.address).toBe(
'0x9c08076187d961f1ed809a9d803fa49037a92039d04f539255072713a180dd5c'
);
expect((await k.getActiveAccount())!.derivationPath).toBe(
"m/44'/784'/0'/0'/0'"
);
expect(account.derivationPath).toBe("m/44'/784'/0'/0'/0'");
});
});

Expand Down
Loading

0 comments on commit d4d234f

Please sign in to comment.