Support keyring metadata in KeyringController #5112
Conversation
|
@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. |
PatrykLucka
force-pushed
the
multi-srp-mvp
branch
from
1ab7570 to
407f96d
Compare
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
|
@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. |
PatrykLucka
force-pushed
the
multi-srp-mvp
branch
2 times, most recently
from
bc69ea1 to
b9ca305
Compare
<!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> The `KeyringController.verifySeedPhrase` method was not included in the mutable methods that lock the controller mutex because it doesn't change the state. Though, if another operation gets somehow overlapped (e.g. a consumer calls `addNewAccount`), the call to `verifySeedPhrase` can potentially fail. To fix this, this PR is moving verifySeedPhrase behind KeyringController's mutex. Since `addNewAccount` internally calls `verifySeedPhrase`, and having a lock on both would create a deadlock, the `verifySeedPhrase` implementation has been moved to an internal method. <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> - **FIXED**: `verifySeedPhrase` is now mutually exclusive - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
PatrykLucka
force-pushed
the
multi-srp-mvp
branch
from
b9ca305 to
83c8a21
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. |
| @@ -1028,7 +1082,7 @@ export class KeyringController extends BaseController< | |||
| } | |||
| const newKeyring = (await this.#newKeyring(KeyringTypes.simple, [ | |||
| privateKey, | |||
| ])) as EthKeyring<Json>; | |||
| ], { id: ulid(), name: '' })) as EthKeyring<Json>; | |||
There was a problem hiding this comment.
It looks like we are always passsing the same object to this.#newKeyring:
{
id: ulid(),
name: '',
}Have you considered moving this directly to #newKeyring, removing the need for an additional method argument?
| async verifySeedPhrase(keyringId?: string): Promise<Uint8Array> { | ||
| return this.#withControllerLock(async () => | ||
| this.#verifySeedPhrase(keyringId), | ||
| ); |
There was a problem hiding this comment.
Hmm, this may not work as intended because this method works with the HD Keyrings only, and all other keyring types will throw errors that are difficult to analyse as they would not be that specific
There was a problem hiding this comment.
We can add a guard to check if the keyring instance is a hd keyring and throw if its any other keyrings.
| async addNewAccount( | ||
| accountCount?: number, | ||
| keyringId?: string, | ||
| ): Promise<string> { |
There was a problem hiding this comment.
Similarly to my comment on verifySeedPhrase, this seems too broad and would break with any non-HD keyring. I believe that the client could add new accounts using withKeyring alone, which makes this change unnecessary
| async getAccounts(keyringId?: string): Promise<string[]> { | ||
| if (keyringId) { | ||
| const keyringIndex = this.state.keyringsMetadata.findIndex( | ||
| (keyring) => keyring.id === keyringId, | ||
| ); | ||
| const keyring = this.state.keyrings[keyringIndex]; | ||
| if (!keyring) { | ||
| throw new Error('Keyring not found'); | ||
| } | ||
| return keyring.accounts; | ||
| } |
There was a problem hiding this comment.
Do you think this is really needed? The client can access all accounts divided by keyring through KeyringController.state and withKeyring
There was a problem hiding this comment.
I think we can remove this
|
|
||
| if (this.state.keyringsMetadata.length > this.#keyrings.length) { | ||
| this.update((state) => { | ||
| // remove metadata from the end of the array to have the same length as the keyrings array | ||
| state.keyringsMetadata = state.keyringsMetadata.slice(0, -1 * (state.keyringsMetadata.length - this.#keyrings.length)); | ||
| }) | ||
| } |
There was a problem hiding this comment.
Hmm, this feels like an unexpected situation where I'm not sure it would be safe to 'just delete the last ones'. Maybe we should just throw an error here?
| if (updatedKeyrings.length < state.keyringsMetadata.length) { | ||
| state.keyringsMetadata = state.keyringsMetadata.slice(0, -1 * (state.keyringsMetadata.length - updatedKeyrings.length)); | ||
| } |
There was a problem hiding this comment.
Hmm, this feels like an unexpected situation where I'm not sure it would be safe to 'just delete the last ones'. Maybe we should just throw an error here?
There was a problem hiding this comment.
Hmm but if we throw an error here, the user would not be able to load the extension.
There was a problem hiding this comment.
That's a good point, but I'm also thinking that when a consumer initialises this KeyringController version coming from an older version we'll have an empty keyringMetadata array, which would create several issues in how these are matched with the related keyring
Perhaps we should have checks in place for both scenarios, and a strategy to realign them for when it happens.
The issue that I see is that if other controllers keep track of these IDs (which could be since keyringMetadata is publicly accessible), they could end up keeping track of unreferenced IDs if we remove / change them
There was a problem hiding this comment.
Also, what happens if a keyring from the middle of the array is removed?
There was a problem hiding this comment.
For the clients, there will be a migration to generate the keyringMetadata for all keyrings.
I've added checks to clear the metadata of a keyring if its removed.
We also probably need a new event to notify controllers of metadata changes, like KeyringController:keyringMetadataChanged.
There was a problem hiding this comment.
For the clients, there will be a migration to generate the keyringMetadata for all keyrings.
If you are referring to a state migration, that would be run before unlocking the wallet so keyrings won't be readable at that point
There was a problem hiding this comment.
The thing i'm not sure how to handle is, if there is a mismatch between the number of keyrings and keyringsMetadata. I don't think there is a reliable way to match the id to the keyring instance, since we are currently only using the index.
Yeah, this is a pain point of this solution along with this other one. I advocated on keeping metadata out of the vault to avoid changing its shape (which is just an array of serialized keyrings at the moment), but perhaps saving metadata into the vault would help with these two issues - something to consider at least
There was a problem hiding this comment.
For the clients, there will be a migration to generate the keyringMetadata for all keyrings.
About this, I don't think there is a way to get a list of available keyrings or to manipulate the keyrings array before the vault is injected into KeyringController via constructor and submitPassword is called with the password typed by the user. So KeyringController will have to deal with the possibility of receiving keyrings with no ID, even just to support older states
There was a problem hiding this comment.
For the clients, there will be a migration to generate the keyringMetadata for all keyrings.
About this, I don't think there is a way to get a list of available keyrings or to manipulate the keyrings array before the vault is injected into KeyringController via constructor and
submitPasswordis called with the password typed by the user. So KeyringController will have to deal with the possibility of receiving keyrings with no ID, even just to support older states
I used the number of keyring types thats available in the accounts controller to determine the number of KeyringMetadata to be created. There should only be one unique instances of each keyring because the multi srp feature isn't live. But youre right, if we want to use KeyringController.state.keyrings it would have to be after submitPassword is called. I think there is a need for this type of migration tool.
There was a problem hiding this comment.
I used the number of keyring types thats available in the accounts controller to determine the number of KeyringMetadata to be created.
That would indeed work but not if the user has multiple imported accounts, since each of them will have its own SimpleKeyring instance
| const index = this.state.keyringsMetadata.findIndex( | ||
| (metadata) => metadata.id === keyringId, | ||
| ); | ||
| return this.#keyrings[index] as EthKeyring<Json>; |
There was a problem hiding this comment.
In the case the ID doesn't find a match, index will be set to -1 and undefined will return instead of a Keyring. Perhaps we should either add undefined as possible return type, or check for keyring existence and throw an error in case it doesn't
There was a problem hiding this comment.
Adding undefined as a possible return makes sense.
There was a problem hiding this comment.
We should add a test case for when a keyring is removed (e.g. when all accounts from a keyring are removed)
There was a problem hiding this comment.
Let's make sure the keyring metadata also bubbles up to the AccountsController events.
I believe this block would need an update for that to happen.
Also, do we expect 1 keyring ID === 1 SRP ?
or would we ever have multiple keyrings derived from 1 SRP?
| * Adds a new account to the default (first) HD seed phrase keyring. | ||
| * | ||
| * @param accountCount - Number of accounts before adding a new one, used to | ||
| * @param keyringId - The id of the keyring to add the account to. | ||
| * make the method idempotent. |
There was a problem hiding this comment.
| * Adds a new account to the default (first) HD seed phrase keyring. | |
| * | |
| * @param accountCount - Number of accounts before adding a new one, used to | |
| * @param keyringId - The id of the keyring to add the account to. | |
| * make the method idempotent. | |
| * Adds a new account to the specified keyring | |
| * or to the default (first) HD seed phrase keyring if none is specified. | |
| * | |
| * @param accountCount - Number of accounts before adding a new one, used to | |
| * make the method idempotent. | |
| * @param keyringId - The id of the keyring to add the account to. |
…move keyringMetadata options in #newKeyring
|
@metamaskbot publish-preview |
|
@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. |
|
|
||
| this.update((state) => { | ||
| state.keyringsMetadata = validKeyringMetadata; | ||
| }); |
There was a problem hiding this comment.
Updating the state here will cause other controllers' state change listeners to run, though since the rest of the KeyringController state is updated atomically only after this wrapped callback executes, other controllers will receive a version of the state where keyringsMetadata has been updated, but not the other state properties (e.g. the removed accounts and keyrings are still there)
There was a problem hiding this comment.
I think there are other instances of this problem on this PR, perhaps we should try if it's possible to move the metadata update to a single place, where also keyrings and the rest of the state are updated together, like here:
| sinon | ||
| .stub(encryptor, 'decrypt') | ||
| .throws(new Error('Invalid password')); |
| jest.mock('ulidx', () => ({ | ||
| ulid: () => 'ULID01234567890ABCDEFGHIJKLMN', | ||
| monotonicFactory: () => () => 'ULID01234567890ABCDEFGHIJKLMN', | ||
| })); |
There was a problem hiding this comment.
Instead of mocking here and assigning the same ID to all keyrings, perhaps we can mock it at test case level so we can test additional functionality
Co-authored-by: Michele Esposito <[email protected]>
Explanation
Currently, we assume that the only HD Keyring is the Primary Keyring. Most methods related to adding accounts or displaying the SRP assume that it refers to the first keyring, which is the Primary Keyring. To enable having more than one SRP, and consequently more than one HD Keyring, this PR adds an array called
keyringsMetadata, which contains an id that can be used to identify all keyrings and a proper place to keep other data we might want to add to each keyring such asname.keyringsMetadatais a separate in order to minimize changes that we need to make to keyrings and the way we process them. We also can keep this data outside of the vault, as it's not storing any sensitive data.References
Related to ADR 0002-keyring-id-and-name.md
Blocks Extension PR
Changelog
@metamask/keyring-controller{ id: string }selector towithKeyringfunctionkeyringIdparam toaddNewAccountfunctionkeyringIdparam toexportSeedPhrasefunctionkeyringIdparam togetAccountsfunctionkeyringIdparam toverifySeedPhrasefunction#getKeyringByIdhelper functionkeyringIdparam to#verifySeedPhrasefunctionmetadataparam to#newKeyringfunctionmetadataparam to#newKeyringfunctionChecklist