feat(multi-srp): add id and typeIndex to hd keyrings #5071
Conversation
PatrykLucka
force-pushed
the
multi-srp-poc
branch
from
07d87a5 to
579728b
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
There was a problem hiding this comment.
I feel like these changes should pass through the EthKeyring type before being handled here with casts as they were exceptions to the rule.
I do think that we should apply the changes to all keyrings, and to the type itself - but it would also be nice to see how we intend to use this changes on the clients to assess the actual necessity of them
| /** | ||
| * Returns the keyring with the given id. | ||
| * | ||
| * @param id - The id of the keyring to return. | ||
| * @returns The keyring with the given id. | ||
| */ | ||
| getKeyringById(id: string): unknown { | ||
| const keyring = this.#keyrings.find( | ||
| (item) => | ||
| (item as EthKeyring<Json> & { opts: { id: string } }).opts.id === id, | ||
| ); | ||
| if (!keyring) { | ||
| throw new Error(KeyringControllerError.KeyringNotFound); | ||
| } | ||
|
|
||
| return keyring; | ||
| } | ||
|
|
There was a problem hiding this comment.
This method is unsafe as clients (consumers) should not be able to access a keyring instance directly. That's the reason why getKeyringsBytype and getKeyringForAccount have been deprecated.
One suggestion could be to make this a # method so that consumers are forced to pass through withKeyring
| */ | ||
| export type KeyringObject = { | ||
| accounts: string[]; | ||
| type: string; | ||
| typeIndex: number; |
There was a problem hiding this comment.
I think that storing the this typeIndex value would make things unnecessarily complicated. Can't we just use the ID to get its index programmatically?
Given that we may have multiple HDKeyrings that are restored with the same order every time the wallet is unlocked (the keyring instances are recreated), if one in the middle of the array would get removed we would have to reassign typeIndex to all other keyrings of the same type, which would make the cost / benefit of this unclear to me
| typeIndex: opts?.typeIndex, | ||
| id: opts?.id, |
There was a problem hiding this comment.
This may be undefined but the types changed in the rest of the controller suggest that these are always defined. This could create unexpected scenarios
| async addNewAccount( | ||
| accountCount?: number, | ||
| keyringId?: string, | ||
| ): Promise<string> { | ||
| return this.#persistOrRollback(async () => { | ||
| const primaryKeyring = this.getKeyringsByType('HD Key Tree')[0] as | ||
| | EthKeyring<Json> | ||
| | undefined; | ||
| if (!primaryKeyring) { | ||
| let selectedKeyring: EthKeyring<Json> | undefined; | ||
| if (keyringId) { | ||
| selectedKeyring = this.getKeyringById(keyringId) as EthKeyring<Json>; | ||
| } else { | ||
| selectedKeyring = this.getKeyringsByType( | ||
| 'HD Key Tree', | ||
| )[0] as EthKeyring<Json>; | ||
| } | ||
| if (!selectedKeyring) { |
There was a problem hiding this comment.
Calling this.withKeyring internally here would avoid extra complexity and would delegate to withKeyring the following responsibilities:
- Keyring selection
- Safeguarding the operation with controller locks
- Handle the case where the keyring is inexistent
| @@ -758,6 +783,15 @@ export class KeyringController extends BaseController< | |||
| }); | |||
| } | |||
|
|
|||
| async createKeyringFromMnemonic(mnemonic: string): Promise<string> { | |||
There was a problem hiding this comment.
Mnemonics are usually handled as UInt8Array for security reasons. Can we do the same here?
| if (type === KeyringTypes.hd) { | ||
| await keyring.deserialize({ | ||
| ...(data ?? {}), | ||
| typeIndex: lastIndexOfType + 1, | ||
| id: ulid(), | ||
| }); | ||
| } else { | ||
| // @ts-expect-error Enforce data type after updating clients | ||
| await keyring.deserialize(data); | ||
| } |
There was a problem hiding this comment.
If in methods like addNewAccount we allow a consumer to reference any kind of keyring by its ID, why we do this for the HD keyring only here?
| @@ -758,6 +783,15 @@ export class KeyringController extends BaseController< | |||
| }); | |||
| } | |||
|
|
|||
| async createKeyringFromMnemonic(mnemonic: string): Promise<string> { | |||
There was a problem hiding this comment.
Why do clients need this function if they can use addNewKeyring? It would be nice to avoid new keyring-specific methods on KeyringController since we are already trying to get rid of the ones that are already in place (e.g. QR-related ones)
| async addNewAccount( | ||
| accountCount?: number, | ||
| keyringId?: string, | ||
| ): Promise<string> { |
There was a problem hiding this comment.
There seems to be no restriction now on what kind of Keyring this operation can be executed on. However, this would not make sense for almost all keyrings we have besides the HD one.
What are the benefits of using an ID, instead of a type and an index?
| async getAccounts(keyringId?: string): Promise<string[]> { | ||
| return this.state.keyrings | ||
| .filter((keyring) => (keyringId ? keyring.id === keyringId : true)) | ||
| .reduce<string[]>( | ||
| (accounts, keyring) => accounts.concat(keyring.accounts), | ||
| [], | ||
| ); |
There was a problem hiding this comment.
Clients could autonomously execute this operation by filtering KeyringController's state already
I did take a look at the client's PRs, but I'm still not convinced that we strictly need these changes to allow clients to operate on multiple HD keyrings. Most of the usages that I spotted were to give clients access to specific keyring instances directly, which is something that we want to avoid since it would bypass KeyringController safeguards. I may have missed some, but I was mainly worried about the removal operation of one of the HD keyrings (e.g. one in the middle) which would change the order and indexes of keyrings, but I don't see this feature implemented, and even if it was, I believe the clients would still be able to reference every single keyring in KeyringController and execute operations on them. The only benefit of this that I see is that we would be able to assign pet names to HD Keyrings which could then be shown to users to identify them, but that's something different than an ID used internally to route operations |
| const [addedAccountAddress] = await selectedKeyring.addAccounts(1); | ||
| await this.verifySeedPhrase(); |
There was a problem hiding this comment.
this verifySeedPhrase call will only verify the first HD keyring, while now we probably want to verify them all
| } else if ('id' in selector) { | ||
| keyring = this.getKeyringById(selector.id) as SelectedKeyring; |
There was a problem hiding this comment.
If a client passes a selector like
{
id: undefined
}this function would execute the operation with the first keyring found with no id
| /** | ||
| * Strip the `id` property from each keyring in the state as it is generated | ||
| * by the `ulid()` function and is not deterministic. | ||
| * | ||
| * @param state - The state to strip the `id` property from. | ||
| * @returns The state with the `id` property stripped from each keyring. | ||
| */ | ||
| function stripKeyringIds(state: KeyringControllerState): Omit< | ||
| KeyringControllerState, | ||
| 'keyrings' | ||
| > & { | ||
| keyrings: Omit<(typeof state.keyrings)[number], 'id'>[]; | ||
| } { | ||
| return { | ||
| ...state, | ||
| keyrings: state.keyrings.map(({ id, ...rest }) => rest), | ||
| }; | ||
| } |
There was a problem hiding this comment.
We could, alternatively, mock the ulid package
Explanation
References
Changelog
@metamask/package-a@metamask/package-bChecklist