Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support keyring metadata in KeyringController #5112

Merged
merged 38 commits into from
Feb 17, 2025
+563 −84
Merged

Support keyring metadata in KeyringController #5112

Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
7f1608a
feat(multi-srp): add id and typeIndex to hd keyrings
PatrykLucka Dec 16, 2024
281d85f
feat: use keyring index to select srp
PatrykLucka Jan 8, 2025
9c88af9
fix: lock `KeyringController` mutex on `verifySeedPhrase` (#5077)
mikesposito Dec 17, 2024
ec9e428
feat(multi-srp): add id and typeIndex to hd keyrings
PatrykLucka Dec 16, 2024
cbb3803
feat: use keyring index to select srp
PatrykLucka Jan 8, 2025
af226d5
feat: add keyringsMetadata
PatrykLucka Jan 13, 2025
3062aeb
feat: keep metadata in state only
PatrykLucka Jan 14, 2025
ad3e967
fix: revert keyringId arguments in getAccounts, and addNewAccount. Re…
montelaidev Jan 22, 2025
3813a19
fix: handle keyringMetadata cleanup when removing emptyKeyrings and c…
montelaidev Jan 22, 2025
c1409bd
fix: remove keyringmetadata misalignement logic
PatrykLucka Feb 5, 2025
8ba574c
fix: build
PatrykLucka Feb 5, 2025
9559258
test(multi-srp): update unit tests coverage
PatrykLucka Feb 5, 2025
40fac06
fix(multi-srp): clear keyringsMetadata on createNewVaultAndRestore
PatrykLucka Feb 6, 2025
bcaa0a3
feat(multi-srp): add keyring metadta to AccountsController keyring ob…
PatrykLucka Feb 7, 2025
0256b1e
chore: add FIXME packages/keyring-controller/src/KeyringController.ts…
PatrykLucka Feb 7, 2025
3265053
chore: fix lint issues
shane-t Feb 7, 2025
1d4430b
fix: lint warnings
PatrykLucka Feb 7, 2025
4645938
chore: prettier fix
shane-t Feb 7, 2025
09c9430
chore: warning thresholds
shane-t Feb 7, 2025
d5981fa
fix(multi-srp): use internal variable instead of property for metada…
PatrykLucka Feb 7, 2025
03c9731
chore(multi-srp): throw mismatch array outside of the update method
PatrykLucka Feb 11, 2025
ddd8737
fix(multi-srp): use push instead of spread operator for keyringsMetadata
PatrykLucka Feb 11, 2025
4fbbd13
Merge branch 'main' into multi-srp-mvp
PatrykLucka Feb 11, 2025
89746ba
Update packages/keyring-controller/src/KeyringController.ts
PatrykLucka Feb 12, 2025
03509bc
Update packages/keyring-controller/src/KeyringController.ts
PatrykLucka Feb 12, 2025
ca5548b
Update packages/keyring-controller/src/KeyringController.ts
PatrykLucka Feb 12, 2025
aebc8a7
test(multi-srp): add remove last account test
PatrykLucka Feb 12, 2025
5022d86
chore(multi-srp): move primary keyring removal protection to removeAc…
PatrykLucka Feb 12, 2025
ae1a759
chore: Improve error handling related to keyring metadata
Gudahtt Feb 12, 2025
cb58d7d
Merge branch 'main' into multi-srp-mvp
PatrykLucka Feb 13, 2025
f671b62
Merge branch 'improved-keyring-metadata-error-handling' into multi-sr…
PatrykLucka Feb 13, 2025
b93bbdf
test(multi-srp): update test coverage config
PatrykLucka Feb 13, 2025
7a6b427
test(multi-srp): fix length mismatch unit test
PatrykLucka Feb 14, 2025
a0a1422
chore(multi-srp): review fixes
PatrykLucka Feb 17, 2025
f823f91
chore(multi-srp): revert AccountsController changes
PatrykLucka Feb 17, 2025
66c3f05
chore(multi-srp): review changes
PatrykLucka Feb 17, 2025
2fafa9d
Merge branch 'main' into multi-srp-mvp
PatrykLucka Feb 17, 2025
03377ae
Merge branch 'main' into multi-srp-mvp
PatrykLucka Feb 17, 2025
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
5 changes: 2 additions & 3 deletions eslint-warning-thresholds.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,13 +243,12 @@
"n/no-unsupported-features/node-builtins": 1
},
"packages/keyring-controller/src/KeyringController.test.ts": {
"import-x/namespace": 16,
"import-x/namespace": 15,
"jest/no-conditional-in-test": 8
},
"packages/keyring-controller/src/KeyringController.ts": {
"@typescript-eslint/no-unsafe-enum-comparison": 5,
"@typescript-eslint/no-unused-vars": 2,
"jsdoc/tag-lines": 1
"@typescript-eslint/no-unused-vars": 2
},
"packages/keyring-controller/tests/mocks/mockKeyring.ts": {
"@typescript-eslint/prefer-readonly": 1
Expand Down
127 changes: 126 additions & 1 deletion packages/accounts-controller/src/AccountsController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import type { SnapControllerState } from '@metamask/snaps-controllers';
import { SnapStatus } from '@metamask/snaps-utils';
import type { CaipChainId } from '@metamask/utils';
import * as uuid from 'uuid';

Check warning on line 25 in packages/accounts-controller/src/AccountsController.test.ts

View workflow job for this annotation

GitHub Actions / Lint, build, and test / Lint (20.x) 

No exported names found in module 'uuid'
import type { V4Options } from 'uuid';

import type {
Expand Down Expand Up @@ -540,6 +540,12 @@
accounts: [mockAccount.address],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
],
};

messenger.publish(
Expand All @@ -564,7 +570,7 @@

messenger.publish(
'KeyringController:stateChange',
{ isUnlocked: true, keyrings: [] },
{ isUnlocked: true, keyrings: [], keyringsMetadata: [] },
[],
);

Expand All @@ -582,6 +588,13 @@
{
accounts: [mockAccount.address, mockAccount2.address],
type: KeyringTypes.hd,
id: '123',
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
],
};
Expand Down Expand Up @@ -620,6 +633,12 @@
accounts: [mockAccount.address, mockAccount2.address],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
],
};
const { accountsController } = setupAccountsController({
initialState: {
Expand Down Expand Up @@ -677,6 +696,16 @@
accounts: [mockAccount3.address, mockAccount4.address],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
{
id: 'mock-id2',
name: 'mock-name2',
},
],
};

const { accountsController } = setupAccountsController({
Expand Down Expand Up @@ -744,6 +773,16 @@
accounts: [mockAccount3.address, mockAccount4.address],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
{
id: 'mock-id2',
name: 'mock-name2',
},
],
};

const { accountsController } = setupAccountsController({
Expand Down Expand Up @@ -790,6 +829,12 @@
],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
],
};
const { accountsController } = setupAccountsController({
initialState: {
Expand Down Expand Up @@ -852,6 +897,12 @@
],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
],
};
const { accountsController } = setupAccountsController({
initialState: {
Expand Down Expand Up @@ -912,6 +963,16 @@
accounts: [mockAccount3.address],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
{
id: 'mock-id2',
name: 'mock-name2',
},
],
};

const { accountsController } = setupAccountsController({
Expand Down Expand Up @@ -948,6 +1009,12 @@
accounts: [mockAccount.address, mockAccount2.address],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
],
};
const { accountsController } = setupAccountsController({
initialState: {
Expand Down Expand Up @@ -1003,6 +1070,12 @@
accounts: [mockAccount.address, mockAccount2.address],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
],
};

messenger.publish(
Expand Down Expand Up @@ -1034,6 +1107,12 @@
accounts: [mockAccount2.address],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
],
};
const { accountsController } = setupAccountsController({
initialState: {
Expand Down Expand Up @@ -1075,6 +1154,12 @@
accounts: [mockAccount.address, mockAccount2.address],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
],
};
const { accountsController } = setupAccountsController({
initialState: {
Expand Down Expand Up @@ -1135,6 +1220,12 @@
accounts: [mockAccount.address, mockAccount2.address],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
],
};
const { accountsController } = setupAccountsController({
initialState: {
Expand Down Expand Up @@ -1208,6 +1299,12 @@
],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
],
};
const { accountsController } = setupAccountsController({
initialState: {
Expand Down Expand Up @@ -1278,6 +1375,12 @@
accounts: [mockAccount.address, mockAccount2.address],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
],
};
messenger.publish(
'KeyringController:stateChange',
Expand Down Expand Up @@ -1322,6 +1425,12 @@
accounts: [mockReinitialisedAccount.address],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
],
};
const { accountsController } = setupAccountsController({
initialState: {
Expand Down Expand Up @@ -1418,6 +1527,12 @@
],
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
],
};
messenger.publish(
'KeyringController:stateChange',
Expand Down Expand Up @@ -2694,6 +2809,16 @@
accounts: simpleAddressess,
},
],
keyringsMetadata: [
{
id: 'mock-id',
name: 'mock-name',
},
{
id: 'mock-id2',
name: 'mock-name2',
},
],
};
};

Expand Down
6 changes: 5 additions & 1 deletion packages/accounts-controller/src/AccountsController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
type KeyringControllerGetKeyringsByTypeAction,
type KeyringControllerGetAccountsAction,
type KeyringControllerStateChangeEvent,
type KeyringMetadata,
KeyringTypes,
} from '@metamask/keyring-controller';
import type { InternalAccount } from '@metamask/keyring-internal-api';
Expand Down Expand Up @@ -213,6 +214,7 @@ export type AccountsControllerMessenger = RestrictedMessenger<
type AddressAndKeyringTypeObject = {
address: string;
type: string;
metadata: KeyringMetadata;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this? I think it has no real use, so I would remove it if possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ccharly I'll have to re-re-read (😅) the PR but I think it could be of use for account syncing?

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be useful indeed 🤔 even though, I'm not a fan of duplicating (kinda like mirroring, but here we truly are duplicating) state everywhere.

However, we must be careful with the metadata field, we do have a schema for the InternalAccount and "native accounts" (or also named "normal accounts") which includes simple + HD keyring accounts, will also be re-created as InternalAccount in the updateAccounts method of this controller.

This controller re-creates internally its list of accounts based on keyring's accounts.

And I think having a "flat" KeyringMetadata here is not ideal. I would maybe add an intermediary field for this, like keyringMetadata maybe? It could also introduces conflicts and overwrites potentially, because of this logic:

BTW, here's the list of "known metadata":

WDYT @montelaidev? @PatrykLucka is this already being used elsewhere in some other PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, the goal is to make it easier to find the keyring of a specific account. Since we can have multiple HD keyrings, it's now easier to query them by id. However, we will soon need to add entropySourceId, which will be essential to support multiple SRPs for snaps.

Copy link
Member

@shane-t shane-t Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

entropySourceId at the moment (though this doesn't exist in our code anywhere, only in our Architecture) is in account.options

The reasoning here is that an account snap can set this during the AccountCreated event (see this line here in "Multi-SRP for preinstalled account snaps"

I'm not sure if this helps, just feel like it's the right time to highlight it

Copy link
Contributor

Choose a reason for hiding this comment

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

I think your comment is on point actually @shane-t! That makes sense for Snap accounts, but to my knowledge, we don't have any use of the options bag for the "normal accounts". So IDK if we'll follow the same logic for those, but this we have to be decided (if not already 🙈).

My comment here, was more about being careful of adding some new information to the metadata object (because of the way InternalAccount are being re-created internally).

Also, after checking the accounts state, I'd say that metadata.keyring.id might be the safest option here (but could be a bit more tricky to implement).

My recommendation here would be to remove this keyringMetadata from the logic of this PR if we don't use it now.

We can always follow-up with a 2nd PR to re-introduce this if needed (but we'll have to discuss and add more tests to make sure nothing gets overwritten or cause conflicts).

WDYT @shane-t @PatrykLucka?

Copy link
Member

@shane-t shane-t Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense. As I understand it though, we have to find some way for @mathieuartu to read the keyring ID for eth-hd-keyring account related events, right?

I might be off here, but the type that's needed in, is whatever type represents the event paylioad

Copy link

Choose a reason for hiding this comment

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

We need some form of metadata about the source of the account in the accountAdded/accountRenamed events to be able to perform account sync properly.
If we can get that information by some other means, please guide us to it.

A second PR would also be acceptable as long as it's within a similar timeframe.
Without this, we won't be able to filter events properly and you'll end up with a new account on your primary SRP every time a new account is added elsewhere.

Copy link
Member

@mikesposito mikesposito Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For now, the goal is to make it easier to find the keyring of a specific account

@PatrykLucka could you elaborate on that? since withKeyring can be used with the { address } selector, isn't the account itself enough to access the keyring it belongs to?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to close up this thread, it has been decided to not inject those metadata for the moment. Account syncing will be addressed later with the addition of entropySource (see SIP-30).

};

const accountsControllerMetadata = {
Expand Down Expand Up @@ -739,13 +741,14 @@ export class AccountsController extends BaseController<
const updatedNormalKeyringAddresses: AddressAndKeyringTypeObject[] = [];
const updatedSnapKeyringAddresses: AddressAndKeyringTypeObject[] = [];

for (const keyring of keyringState.keyrings) {
for (const [index, keyring] of keyringState.keyrings.entries()) {
if (keyring.type === KeyringTypes.snap) {
updatedSnapKeyringAddresses.push(
...keyring.accounts.map((address) => {
return {
address,
type: keyring.type,
metadata: keyringState.keyringsMetadata[index],
};
}),
);
Expand All @@ -755,6 +758,7 @@ export class AccountsController extends BaseController<
return {
address,
type: keyring.type,
metadata: keyringState.keyringsMetadata[index],
};
}),
);
Expand Down
6 changes: 3 additions & 3 deletions packages/keyring-controller/jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, {
// An object that configures minimum threshold enforcement for coverage results
coverageThreshold: {
global: {
branches: 94.26,
branches: 93.56,
functions: 100,
lines: 98.96,
statements: 98.98,
lines: 98.73,
statements: 98.74,
},
},

Expand Down
9 changes: 7 additions & 2 deletions packages/keyring-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@
"@metamask/utils": "^11.1.0",
"async-mutex": "^0.5.0",
"ethereumjs-wallet": "^1.0.1",
"immer": "^9.0.6"
"immer": "^9.0.6",
"ulid": "^2.3.0"
},
"devDependencies": {
"@ethereumjs/common": "^3.2.0",
Expand Down Expand Up @@ -89,6 +90,10 @@
"registry": "https://registry.npmjs.org/"
},
"lavamoat": {
"allowScripts": {}
"allowScripts": {
"@lavamoat/preinstall-always-fail": false,
"ethereumjs-wallet>ethereum-cryptography>keccak": false,
"ethereumjs-wallet>ethereum-cryptography>secp256k1": false
}
}
}
Loading
Loading