diff --git a/packages/keyring-api/src/api/account.ts b/packages/keyring-api/src/api/account.ts index f0cc4661..0a09a66a 100644 --- a/packages/keyring-api/src/api/account.ts +++ b/packages/keyring-api/src/api/account.ts @@ -74,7 +74,7 @@ export const KeyringAccountStruct = object({ address: string(), /** - * Account supported scopes (CAIP-2 chain IDs). + * Account supported scopes (CAIP-2 chain IDs or CAIP-2 namespaces). */ scopes: nonempty(array(union([CaipNamespaceStruct, CaipChainIdStruct]))), diff --git a/packages/keyring-api/src/btc/types.test.ts b/packages/keyring-api/src/btc/types.test.ts index 480a561a..01e9ea45 100644 --- a/packages/keyring-api/src/btc/types.test.ts +++ b/packages/keyring-api/src/btc/types.test.ts @@ -1,4 +1,20 @@ -import { BtcP2wpkhAddressStruct } from './types'; +import { BtcScopes } from './constants'; +import type { BtcP2wpkhAccount } from './types'; +import { + BtcMethod, + BtcP2wpkhAccountStruct, + BtcP2wpkhAddressStruct, +} from './types'; +import { BtcAccountType } from '../api'; + +const MOCK_ACCOUNT = { + id: '55583f38-d81b-48f8-8494-fc543c2b5c95', + type: BtcAccountType.P2wpkh, + address: 'bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4', + methods: [BtcMethod.SendBitcoin], + options: {}, + scopes: [BtcScopes.Mainnet], +}; describe('types', () => { describe('BtcP2wpkhAddressStruct', () => { @@ -37,5 +53,25 @@ describe('types', () => { `${errorPrefix}: No separator character for ${address}`, ); }); + + it('throws an error if there are multiple scopes', () => { + const account: BtcP2wpkhAccount = { + ...MOCK_ACCOUNT, + scopes: [BtcScopes.Mainnet, BtcScopes.Testnet], + }; + expect(() => BtcP2wpkhAccountStruct.assert(account)).toThrow( + 'At path: scopes -- Expected a array with a length of `1` but received one with a length of `2`', + ); + }); + + it('throws an error if there is no scope', () => { + const account: BtcP2wpkhAccount = { + ...MOCK_ACCOUNT, + scopes: [], + }; + expect(() => BtcP2wpkhAccountStruct.assert(account)).toThrow( + 'At path: scopes -- Expected a array with a length of `1` but received one with a length of `0`', + ); + }); }); }); diff --git a/packages/keyring-api/src/btc/types.ts b/packages/keyring-api/src/btc/types.ts index f610293d..d24f53cd 100644 --- a/packages/keyring-api/src/btc/types.ts +++ b/packages/keyring-api/src/btc/types.ts @@ -1,6 +1,14 @@ import { object } from '@metamask/keyring-utils'; import type { Infer } from '@metamask/superstruct'; -import { string, array, enums, refine, literal } from '@metamask/superstruct'; +import { + string, + array, + enums, + refine, + literal, + size, +} from '@metamask/superstruct'; +import { CaipChainIdStruct } from '@metamask/utils'; import { bech32 } from 'bech32'; import { BtcAccountType, KeyringAccountStruct } from '../api'; @@ -41,6 +49,13 @@ export const BtcP2wpkhAccountStruct = object({ */ type: literal(`${BtcAccountType.P2wpkh}`), + /** + * Account supported scope (CAIP-2 chain ID). + * + * NOTE: We consider a Bitcoin address to be valid on only 1 network at time. + */ + scopes: size(array(CaipChainIdStruct), 1), + /** * Account supported methods. */ diff --git a/packages/keyring-api/src/eth/constants.ts b/packages/keyring-api/src/eth/constants.ts index 2ba369d7..c37e0da0 100644 --- a/packages/keyring-api/src/eth/constants.ts +++ b/packages/keyring-api/src/eth/constants.ts @@ -6,4 +6,5 @@ export enum EthScopes { Namespace = 'eip155', Mainnet = 'eip155:1', + Testnet = 'eip155:11155111', } diff --git a/packages/keyring-api/src/eth/types.ts b/packages/keyring-api/src/eth/types.ts index e52993fa..20b85d0f 100644 --- a/packages/keyring-api/src/eth/types.ts +++ b/packages/keyring-api/src/eth/types.ts @@ -1,6 +1,7 @@ import { object, definePattern } from '@metamask/keyring-utils'; import type { Infer } from '@metamask/superstruct'; import { nonempty, array, enums, literal } from '@metamask/superstruct'; +import { CaipChainIdStruct } from '@metamask/utils'; import { EthScopes } from '.'; import { EthAccountType, KeyringAccountStruct } from '../api'; @@ -82,6 +83,11 @@ export const EthErc4337AccountStruct = object({ */ type: literal(`${EthAccountType.Erc4337}`), + /** + * Account supported scopes (CAIP-2 chain IDs). + */ + scopes: nonempty(array(CaipChainIdStruct)), + /** * Account supported methods. */ diff --git a/packages/keyring-api/src/sol/types.ts b/packages/keyring-api/src/sol/types.ts index 64dcaa66..da8caffd 100644 --- a/packages/keyring-api/src/sol/types.ts +++ b/packages/keyring-api/src/sol/types.ts @@ -1,6 +1,7 @@ import { object, definePattern } from '@metamask/keyring-utils'; import type { Infer } from '@metamask/superstruct'; -import { array, enums, literal } from '@metamask/superstruct'; +import { array, enums, literal, nonempty } from '@metamask/superstruct'; +import { CaipChainIdStruct } from '@metamask/utils'; import { KeyringAccountStruct, SolAccountType } from '../api'; @@ -35,6 +36,11 @@ export const SolDataAccountStruct = object({ */ type: literal(`${SolAccountType.DataAccount}`), + /** + * Account supported scopes (CAIP-2 chain IDs). + */ + scopes: nonempty(array(CaipChainIdStruct)), + /** * Account supported methods. */ diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts index 1c61145d..4f09d150 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts @@ -117,7 +117,7 @@ describe('SnapKeyring', () => { }; const ethEoaAccount3 = { id: 'c6697bcf-5710-4751-a1cb-340e4b50617a', - address: '0xab1G3q98V7C67T9103g30C0417610237A137d763'.toLowerCase(), + address: '0xf7bDe8609231033c69E502C08f85153f8A1548F2'.toLowerCase(), options: {}, methods: ETH_EOA_METHODS, scopes: [EthScopes.Namespace], @@ -128,7 +128,7 @@ describe('SnapKeyring', () => { address: '0x2f15b30952aebe0ed5fdbfe5bf16fb9ecdb31d9a'.toLowerCase(), options: {}, methods: ETH_4337_METHODS, - scopes: [EthScopes.Namespace], + scopes: [EthScopes.Testnet], type: EthAccountType.Erc4337, }; const btcP2wpkhAccount = { @@ -163,7 +163,7 @@ describe('SnapKeyring', () => { // For unknown accounts, we consider them as EVM EOA for now, so just re-use the // same scopes. scopes: [EthScopes.Namespace], - // This should be really possible to create such account, but since we potentially + // This should not be really possible to create such account, but since we potentially // migrate data upon the Snap keyring initialization, we want to cover edge-cases // like this one to avoid crashing and blocking everything... type: 'unknown:type' as KeyringAccount['type'], @@ -312,7 +312,7 @@ describe('SnapKeyring', () => { params: { account: { ...(ethEoaAccount1 as unknown as KeyringAccount), - address: '0x0', + address: ethEoaAccount2.address, }, }, }), @@ -447,7 +447,7 @@ describe('SnapKeyring', () => { }, }), ).rejects.toThrow( - 'Account scopes is required for non-EVM and ERC4337 accounts', + 'At path: scopes -- Expected an array value, but received: undefined', ); }); @@ -633,7 +633,7 @@ describe('SnapKeyring', () => { it('fails when the EthMethod is not supported after update', async () => { // Update first account to remove `EthMethod.PersonalSign` - let updatedMethods: EthMethod[] = Object.values(EthMethod).filter( + let updatedMethods: EthMethod[] = Object.values(ETH_EOA_METHODS).filter( (method) => method !== EthMethod.PersonalSign, ); expect( @@ -656,7 +656,7 @@ describe('SnapKeyring', () => { `Method '${EthMethod.PersonalSign}' not supported for account ${ethEoaAccount1.address}`, ); // Restore `EthMethod.PersonalSign` and remove `EthMethod.SignTransaction` - updatedMethods = Object.values(EthMethod).filter( + updatedMethods = Object.values(ETH_EOA_METHODS).filter( (method) => method !== EthMethod.SignTransaction, ); expect( @@ -723,7 +723,7 @@ describe('SnapKeyring', () => { params: { account }, }), ).rejects.toThrow( - 'Account scopes is required for non-EVM and ERC4337 accounts', + 'At path: scopes -- Expected an array value, but received: undefined', ); }); @@ -740,7 +740,7 @@ describe('SnapKeyring', () => { params: { account }, }), ).rejects.toThrow( - 'Account scopes is required for non-EVM and ERC4337 accounts', + 'At path: scopes -- Expected an array value, but received: undefined', ); }); }); diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index 4783a5c5..d76d4b40 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.ts @@ -43,6 +43,7 @@ import { import { EventEmitter } from 'events'; import { v4 as uuid } from 'uuid'; +import { transformAccount } from './account'; import { DeferredPromise } from './DeferredPromise'; import { AccountCreatedEventStruct, @@ -52,11 +53,7 @@ import { RequestRejectedEventStruct, } from './events'; import { projectLogger as log } from './logger'; -import { - isAccountV1, - migrateAccountV1, - transformAccountV1, -} from './migrations'; +import { isAccountV1, migrateAccountV1 } from './migrations'; import { SnapIdMap } from './SnapIdMap'; import type { SnapKeyringEvents, @@ -208,9 +205,8 @@ export class SnapKeyring extends EventEmitter { displayConfirmation, } = message.params; - // To keep the retro-compatibility with older keyring-api versions, we mark some fields - // as optional and provide them here if they are missing. - const account = transformAccountV1(newAccountFromEvent); + // Potentially migrate the account. + const account = transformAccount(newAccountFromEvent); // The UI still uses the account address to identify accounts, so we need // to block the creation of duplicate accounts for now to prevent accounts @@ -258,9 +254,8 @@ export class SnapKeyring extends EventEmitter { this.#accounts.get(snapId, newAccountFromEvent.id) ?? throwError(`Account '${newAccountFromEvent.id}' not found`); - // To keep the retro-compatibility with older keyring-api versions, we mark some fields - // as optional and provide them here if they are missing. - const newAccount = transformAccountV1(newAccountFromEvent); + // Potentially migrate the account. + const newAccount = transformAccount(newAccountFromEvent); // The address of the account cannot be changed. In the future, we will // support changing the address of an account since it will be required to diff --git a/packages/keyring-snap-bridge/src/account.test.ts b/packages/keyring-snap-bridge/src/account.test.ts new file mode 100644 index 00000000..d56d62cc --- /dev/null +++ b/packages/keyring-snap-bridge/src/account.test.ts @@ -0,0 +1,18 @@ +import type { KeyringAccount } from '@metamask/keyring-api'; + +import { transformAccount } from './account'; + +describe('account', () => { + it('throws for unknown account type', () => { + const unknownAccount = { + // This should not be really possible to create such account, but since we potentially + // migrate data upon the Snap keyring initialization, we want to cover edge-cases + // like this one to avoid crashing and blocking everything... + type: 'unknown:type', + } as unknown as KeyringAccount; // Just testing the default case. + + expect(() => transformAccount(unknownAccount)).toThrow( + "Unknown account type: 'unknown:type'", + ); + }); +}); diff --git a/packages/keyring-snap-bridge/src/account.ts b/packages/keyring-snap-bridge/src/account.ts index 74c7bae1..69d0d974 100644 --- a/packages/keyring-snap-bridge/src/account.ts +++ b/packages/keyring-snap-bridge/src/account.ts @@ -1,5 +1,17 @@ -import { KeyringAccountStruct } from '@metamask/keyring-api'; -import { omit, type Infer } from '@metamask/superstruct'; +import type { KeyringAccount, KeyringAccountType } from '@metamask/keyring-api'; +import { + BtcAccountType, + BtcP2wpkhAccountStruct, + EthAccountType, + EthEoaAccountStruct, + EthErc4337AccountStruct, + KeyringAccountStruct, + SolAccountType, + SolDataAccountStruct, +} from '@metamask/keyring-api'; +import { assert, omit, type Infer } from '@metamask/superstruct'; + +import { isAccountV1, transformAccountV1 } from './migrations'; /** * A `KeyringAccount` with some optional fields which can be used to keep @@ -8,3 +20,60 @@ import { omit, type Infer } from '@metamask/superstruct'; export const KeyringAccountV1Struct = omit(KeyringAccountStruct, ['scopes']); export type KeyringAccountV1 = Infer; + +/** + * Assert that an account-like object matches its actual account type. + * + * @param account - The account-like object. + * @returns The account as normal `KeyringAccount`. + */ +export function assertKeyringAccount< + Account extends { type: KeyringAccountType }, +>(account: Account): KeyringAccount { + // TODO: We should use a `selectiveUnion` for this and probably use it to define + // the `KeyringAccount`. This would also required to have a "generic `KeyringAccount`" + // definition. + switch (account.type) { + case BtcAccountType.P2wpkh: { + assert(account, BtcP2wpkhAccountStruct); + return account; + } + case SolAccountType.DataAccount: { + assert(account, SolDataAccountStruct); + return account; + } + case EthAccountType.Erc4337: { + assert(account, EthErc4337AccountStruct); + return account; + } + case EthAccountType.Eoa: { + assert(account, EthEoaAccountStruct); + return account; + } + default: { + // For now, we cannot much more than this (this should also, never happen)! + // NOTE: We could use a "generic `KeyringAccount` type" here though. + throw new Error(`Unknown account type: '${account.type}'`); + } + } +} + +/** + * Transform any versionned account to a `KeyringAccount`. + * + * @param accountToTransform - The account to transform. + * @returns A valid transformed `KeyringAccount`. + */ +export function transformAccount( + // eslint-disable-next-line @typescript-eslint/no-redundant-type-constituents + accountToTransform: KeyringAccountV1 | KeyringAccount, +): KeyringAccount { + // To keep the retro-compatibility with older keyring-api versions, we identify the account's + // version and transform it to the latest `KeyringAccount` representation. + const account = isAccountV1(accountToTransform) + ? transformAccountV1(accountToTransform) + : accountToTransform; + + // We still assert that the converted account is valid according to their account's type. + return assertKeyringAccount(account); +} diff --git a/packages/keyring-snap-bridge/src/migrations/v1.ts b/packages/keyring-snap-bridge/src/migrations/v1.ts index d99e362c..b69b4709 100644 --- a/packages/keyring-snap-bridge/src/migrations/v1.ts +++ b/packages/keyring-snap-bridge/src/migrations/v1.ts @@ -10,7 +10,11 @@ import { import { isBtcMainnetAddress } from '@metamask/keyring-utils'; import { is } from '@metamask/superstruct'; -import { KeyringAccountV1Struct, type KeyringAccountV1 } from '../account'; +import { + assertKeyringAccount, + KeyringAccountV1Struct, + type KeyringAccountV1, +} from '../account'; /** * Checks if an account is an `KeyringAccount` v1. @@ -40,10 +44,11 @@ export function getScopesForAccountV1(accountV1: KeyringAccountV1): string[] { } case EthAccountType.Erc4337: { // EVM Erc4337 account - // NOTE: A Smart Contract account might not be compatible with every chain, but we still use - // "generic" scope for now. Also, there's no official Snap as of today that uses this account type. So - // this case should never happen. - return [EthScopes.Namespace]; + // NOTE: A Smart Contract account might not be compatible with every chain, in this case we just default + // to testnet since we cannot really "guess" it from here. + // Also, there's no official Snap as of today that uses this account type. So this case should never happen + // in production. + return [EthScopes.Testnet]; } case BtcAccountType.P2wpkh: { // Bitcoin uses different accounts for testnet and mainnet @@ -77,24 +82,20 @@ export function transformAccountV1( ): KeyringAccount { const { type } = accountV1; - if (!isAccountV1(accountV1)) { - // Nothing to do in this case. - return accountV1 as KeyringAccount; - } - + // EVM EOA account are compatible with any EVM networks, and we use CAIP-2 + // namespaces when the scope relates to ALL chains (from that namespace). + // So we can automatically inject a valid `scopes` for this, but not for + // other kind of accounts. if (type === EthAccountType.Eoa) { - // EVM EOA account are compatible with any EVM networks, and we use CAIP-2 - // namespaces when the scope relates to ALL chains (from that namespace). return { ...accountV1, scopes: getScopesForAccountV1(accountV1), }; } - // For all non-EVM Snaps and ERC4337 Snaps, the scopes is required. - throw new Error( - `Account scopes is required for non-EVM and ERC4337 accounts`, - ); + // For all other non-EVM and ERC4337 Snap accounts, the `scopes` is required, and + // each `*AccountStruct` should assert that automatically. + return assertKeyringAccount(accountV1); } /**