Skip to content
This repository was archived by the owner on Oct 7, 2024. It is now read-only.
Closed
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
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
"dependencies": {
"@ethereumjs/tx": "^4.1.2",
"@metamask/eth-sig-util": "^5.1.0",
"@metamask/keyring-api": "^0.1.3",
"@metamask/snaps-controllers": "^0.35.2-flask.1",
"@metamask/keyring-api": "^0.2.1",
"@metamask/snaps-controllers": "^0.38.2-flask.1",
"@metamask/utils": "^6.1.0",
"@types/uuid": "^9.0.1",
"superstruct": "^1.0.3",
Expand Down
83 changes: 67 additions & 16 deletions src/SnapKeyring.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import { TransactionFactory } from '@ethereumjs/tx';
import { KeyringAccount } from '@metamask/keyring-api';
import {
EthMethod,
EthAccountType,
InternalAccount,
} from '@metamask/keyring-api';
Comment on lines +2 to +6
Copy link
Member Author

Choose a reason for hiding this comment

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

The tests regarding serialize and deserialize are not working currently because of the data type change.

Copy link
Member Author

Choose a reason for hiding this comment

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

@danroc @montelaidev regarding our conversation earlier today, when should the metadata attribute be added to the accounts object to comply with the InternalAccount? And what's the source of data for the fields snap and name?

Copy link
Contributor

Choose a reason for hiding this comment

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

snap metadata is filled by the SnapKeyring (this repo), the name should be filled by the AccountsController.

import { SnapController } from '@metamask/snaps-controllers';

import { KeyringState, SnapKeyring } from '.';
Expand All @@ -17,18 +21,16 @@ describe('SnapKeyring', () => {
{
id: 'b05d918a-b37c-497a-bb28-3d15c0d56b7a',
address: '0xC728514Df8A7F9271f4B7a4dd2Aa6d2D723d3eE3',
name: 'Account 1',
options: null,
supportedMethods: ['personal_sign', 'eth_sendTransaction'],
type: 'eip155:eoa',
options: {},
methods: [EthMethod.PersonalSign],
type: EthAccountType.Eoa,
},
{
id: '33c96b60-2237-488e-a7bb-233576f3d22f',
address: '0x34b13912eAc00152bE0Cb409A301Ab8E55739e63',
name: 'Account 2',
options: null,
supportedMethods: ['eth_sendTransaction', 'eth_signTypedData'],
type: 'eip155:eoa',
options: {},
methods: [EthMethod.SignTypedData],
type: EthAccountType.Eoa,
},
] as const;

Expand All @@ -38,7 +40,8 @@ describe('SnapKeyring', () => {
mockSnapController.handleRequest.mockResolvedValue(accounts);
await keyring.handleKeyringSnapMessage(snapId, {
method: 'createAccount',
params: { account: account as unknown as KeyringAccount },
// @ts-expect-error Check https://github.com/ianstormtaylor/superstruct/issues/983
params: { account: account as unknown as InternalAccount },
Comment on lines +43 to +44
Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment we're suppressing this error until it's fixed in @metamask/utils.

Type 'undefined' is not assignable to type 'Json'.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gantunesr It should be fixed in @metamask/utils in 8.1.0: #69

});
}
});
Expand All @@ -48,7 +51,9 @@ describe('SnapKeyring', () => {
const result = await keyring.handleKeyringSnapMessage(snapId, {
method: 'listAccounts',
});
expect(result).toStrictEqual(accounts);
expect(result).toStrictEqual(
accounts.map((a) => a.address.toLowerCase()),
);
});

it('should fail if the method is not supported', async () => {
Expand All @@ -60,7 +65,10 @@ describe('SnapKeyring', () => {
});

it('should submit an async request and return the result', async () => {
mockSnapController.handleRequest.mockResolvedValue({ pending: true });
mockSnapController.handleRequest.mockResolvedValue({
pending: true,
redirect: 'https://mock-url.com',
});
const requestPromise = keyring.signPersonalMessage(
accounts[0].address,
'hello',
Expand All @@ -82,7 +90,9 @@ describe('SnapKeyring', () => {
describe('getAccounts', () => {
it('should return all account addresses', async () => {
const addresses = await keyring.getAccounts();
expect(addresses).toStrictEqual(accounts.map((a) => a.address));
expect(addresses).toStrictEqual(
accounts.map((a) => a.address.toLowerCase()),
);
expect(mockSnapController.handleRequest).toHaveBeenCalledWith({
handler: 'onRpcRequest',
origin: 'metamask',
Expand Down Expand Up @@ -152,7 +162,7 @@ describe('SnapKeyring', () => {
});

describe('signTransaction', () => {
it('should sign a transaction synchronously', async () => {
it('should sign a ethereum transaction synchronously', async () => {
const mockTx = {
data: '0x0',
gasLimit: '0x26259fe',
Expand All @@ -178,6 +188,43 @@ describe('SnapKeyring', () => {
const signature = await keyring.signTransaction(accounts[0].address, tx);
expect(signature).toStrictEqual(expectedSignedTx);
});

it('should sign a transaction synchronously and return a userOperation', async () => {
const mockTx = {
data: '0x0',
gasLimit: '0x26259fe',
gasPrice: '0x1',
nonce: '0xfffffffe',
to: '0xccccccccccccd000000000000000000000000000',
value: '0x1869e',
chainId: '0x1',
type: '0x00',
};
const mockSignedTx = {
userOp: {
sender: accounts[0].address,
nonce: '0x0',
initCode: '0x',
callData: '0x',
callGasLimit: '0x0',
verificationGasLimit: '0x2DC6C0',
preVerificationGas: '0x2DC6C0',
maxFeePerGas: '0x0',
maxPriorityFeePerGas: '0x3B9ACA00',
paymasterAndData: '0x',
signature: '0x',
},
userOpHash: '0x0',
};
const tx = TransactionFactory.fromTxData(mockTx);
const expectedSignedTx = mockSignedTx;
mockSnapController.handleRequest.mockResolvedValue({
pending: false,
result: mockSignedTx,
});
const signature = await keyring.signTransaction(accounts[0].address, tx);
expect(signature).toStrictEqual(expectedSignedTx);
});
});

describe('signPersonalMessage', () => {
Expand Down Expand Up @@ -278,14 +325,18 @@ describe('SnapKeyring', () => {
it('should remove an account', async () => {
mockSnapController.handleRequest.mockResolvedValue(null);
await keyring.removeAccount(accounts[0].address);
expect(await keyring.getAccounts()).toStrictEqual([accounts[1].address]);
expect(await keyring.getAccounts()).toStrictEqual([
accounts[1].address.toLowerCase(),
]);
});

it('should remove the account and warn if snap fails', async () => {
const spy = jest.spyOn(console, 'error').mockImplementation();
mockSnapController.handleRequest.mockRejectedValue('error');
await keyring.removeAccount(accounts[0].address);
expect(await keyring.getAccounts()).toStrictEqual([accounts[1].address]);
expect(await keyring.getAccounts()).toStrictEqual([
accounts[1].address.toLowerCase(),
]);
expect(console.error).toHaveBeenCalledWith(
'Account "0xC728514Df8A7F9271f4B7a4dd2Aa6d2D723d3eE3" may not have been removed from snap "local:snap.mock":',
'error',
Expand Down
71 changes: 57 additions & 14 deletions src/SnapKeyring.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { TransactionFactory, TypedTransaction } from '@ethereumjs/tx';
import { TypedDataV1, TypedMessage } from '@metamask/eth-sig-util';
import {
KeyringSnapControllerClient,
KeyringAccount,
KeyringAccountStruct,
KeyringSnapControllerClient,
InternalAccount,
InternalAccountStruct,
} from '@metamask/keyring-api';
import { SnapController } from '@metamask/snaps-controllers';
import { Json } from '@metamask/utils';
import EventEmitter from 'events';
import { EventEmitter } from 'events';
import { assert, object, string, record, Infer } from 'superstruct';
import { v4 as uuid } from 'uuid';

Expand All @@ -19,7 +20,7 @@ import { strictMask, toJson, unique } from './util';
export const SNAP_KEYRING_TYPE = 'Snap Keyring';

export const KeyringStateStruct = object({
addressToAccount: record(string(), KeyringAccountStruct),
addressToAccount: record(string(), InternalAccountStruct),
addressToSnapId: record(string(), string()),
});

Expand All @@ -35,7 +36,7 @@ export class SnapKeyring extends EventEmitter {

#snapClient: KeyringSnapControllerClient;

#addressToAccount: CaseInsensitiveMap<KeyringAccount>;
#addressToAccount: CaseInsensitiveMap<InternalAccount>;

#addressToSnapId: CaseInsensitiveMap<string>;

Expand Down Expand Up @@ -75,6 +76,7 @@ export class SnapKeyring extends EventEmitter {
// Don't call the snap back to list the accounts. The main use case for
// this method is to allow the snap to verify if the keyring's state is
// in sync with the snap's state.
// @ts-expect-error Check https://github.com/ianstormtaylor/superstruct/issues/983
return [...this.#addressToAccount.values()].filter(
(account) => this.#addressToSnapId.get(account.address) === snapId,
);
Expand Down Expand Up @@ -198,18 +200,22 @@ export class SnapKeyring extends EventEmitter {
address: string,
transaction: TypedTransaction,
_opts = {},
): Promise<TypedTransaction> {
): Promise<Json | TypedTransaction> {
const tx = toJson({
...transaction.toJSON(),
type: transaction.type,
chainId: transaction.common.chainId().toString(),
});

const signedTx = await this.#submitRequest(address, 'eth_sendTransaction', [
address,
tx,
]);

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
if (signedTx?.userOp) {
return signedTx;
}
return TransactionFactory.fromTxData(signedTx as any);
}

Expand Down Expand Up @@ -316,12 +322,45 @@ export class SnapKeyring extends EventEmitter {
}

/**
* Syncs all accounts for all snaps.
* List all accounts.
*
* @param sync - Whether to sync accounts with the snaps.
* @returns All accounts.
*/
async listAccounts(sync: boolean): Promise<InternalAccount[]> {
if (sync) {
await this.#syncAllSnapsAccounts();
}

return [...this.#addressToAccount.values()].map((account) => {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const snapId = this.#addressToSnapId.get(account.address)!;

return {
...account,
address: account.address.toLowerCase(),
metadata: {
name: 'account',
snap: {
id: snapId,
enabled: true,
name: 'snap',
},
keyring: {
type: this.type,
},
},
};
});
}

/**
* Syncs all accounts from all snaps.
*
* @param extraSnapIds - Extra snap IDs to sync accounts for.
*/
async #syncAllSnapsAccounts(...extraSnapIds: string[]): Promise<void> {
const snapIds = [...this.#addressToSnapId.values()].concat(extraSnapIds);
const snapIds = extraSnapIds.concat(...this.#addressToSnapId.values());
for (const snapId of unique(snapIds)) {
try {
await this.#syncSnapAccounts(snapId);
Expand Down Expand Up @@ -364,7 +403,7 @@ export class SnapKeyring extends EventEmitter {
* found.
*/
#resolveAddress(address: string): {
account: KeyringAccount;
account: InternalAccount;
snapId: string;
} {
const account = this.#addressToAccount.get(address);
Expand Down Expand Up @@ -398,18 +437,22 @@ export class SnapKeyring extends EventEmitter {
* @returns All accounts associated with the given snap ID.
*/
#getAccountsBySnapId(snapId: string): KeyringAccount[] {
return Object.values(this.#addressToAccount).filter(
return [...this.#addressToAccount.values()].filter(
(account) => this.#addressToSnapId.get(account.address) === snapId,
);
}

/**
* Add an account to the internal maps.
*
* @param account - The account to be added.
* @param snapAccount - The account to be added.
* @param snapId - The snap ID of the account.
*/
#addAccountToMaps(account: KeyringAccount, snapId: string): void {
#addAccountToMaps(snapAccount: KeyringAccount, snapId: string): void {
const account = {
...snapAccount,
address: snapAccount.address.toLowerCase(),
};
this.#addressToAccount.set(account.address, account);
this.#addressToSnapId.set(account.address, snapId);
}
Expand All @@ -419,7 +462,7 @@ export class SnapKeyring extends EventEmitter {
*
* @param account - The account to be removed.
*/
#removeAccountFromMaps(account: KeyringAccount): void {
#removeAccountFromMaps(account: InternalAccount): void {
this.#addressToAccount.delete(account.address);
this.#addressToSnapId.delete(account.address);
}
Expand Down
Loading