feat: add `multichain-account-service` (readonly) #6141

ccharly · 2025-07-17T16:09:50Z

Explanation

Introducing a new service to support multichain accounts/wallets.

References

N/A

Changelog

N/A

Checklist

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 communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

socket-security · 2025-07-17T16:54:22Z

No dependency changes detected. Learn more about Socket for GitHub.

👍 No dependency changes detected in pull request

package.json

shane-t · 2025-07-18T11:03:47Z

packages/multichain-account-controller/src/providers/BaseAccountProvider.ts

+    const accounts: Bip44Account<InternalAccount>[] = [];
+
+    for (const account of this.messenger.call(
+      'AccountsController:listMultichainAccounts',


It might be worth adding a comment saying what this actually does (until such time as the action is changed)
i.e. it fetches all internal accounts

Agreed, some JS doc would be good here.

Just added a small comment explaining what the action does, and also added a comment to explain that we should probably rename this at some point!

Fixed here: de60d19

cryptodev-2s

The work looks good to me, I left some comments/suggestions

packages/multichain-account-controller/src/MultichainAccountController.ts

packages/multichain-account-controller/tsconfig.build.json

packages/multichain-account-controller/tsconfig.json

Co-authored-by: cryptodev-2s <[email protected]>

hmalik88 · 2025-07-18T12:58:40Z

packages/multichain-account-controller/src/MultichainAccountController.ts

+    const entropySources = [];
+    for (const keyring of keyrings) {
+      if (keyring.type === (KeyringTypes.hd as string)) {
+        entropySources.push(keyring.metadata.id);
+      }
+    }


nit: I think you can use a reduce here and condense this code block.

Yup, I did refactor this a bit indeed 😅 I saw that when handling KeyringController:stateChange, so it should be simpler now.

cryptodev-2s · 2025-07-18T15:32:21Z

@ccharly What do you think about renaming this to a Service instead of a Controller?

ccharly · 2025-07-18T15:36:23Z

@ccharly What do you think about renaming this to a Service instead of a Controller?

Well, that was our initial idea with @danroc actually! 😄 I just thought that *Service were mostly used for external data providers.

But yes, this "controller" will remain stateless, so MultichainAccountService makes sense IMO. Would that match our naming convention @cryptodev-2s? If yes, I'd be happy to change the name 👍

Just to be clear, this controller is only using "internal data" (InternalAccount and other account-related data), but it does not interface with any remote/external API.

cryptodev-2s · 2025-07-18T15:42:17Z

@ccharly What do you think about renaming this to a Service instead of a Controller?

Well, that was our initial idea with @danroc actually! 😄 I just thought that *Service were mostly used for external data providers.

But yes, this "controller" will remain stateless, so MultichainAccountService makes sense IMO. Would that match our naming convention @cryptodev-2s? If yes, I'd be happy to change the name 👍

Just to be clear, this controller is only using "internal data" (InternalAccount and other account-related data), but it does not interface with any remote/external API.

Yes, that aligns well with our current definition of what a Service could be. Glad we’re on the same page, feel free to proceed with the rename.

ccharly · 2025-07-18T16:04:19Z

Bug: Bip44Account Function Error Handling

The isBip44Account function may throw a TypeError if account.options is undefined when attempting to access account.options.entropy. A defensive check for account.options?.entropy is recommended. Additionally, the function uses console.warn directly, which should be replaced with a proper logging mechanism.

packages/multichain-account-service/src/providers/BaseAccountProvider.ts#L26-L33

core/packages/multichain-account-service/src/providers/BaseAccountProvider.ts

Lines 26 to 33 in b26e629

): account is Bip44Account<Account> {

if (

!account.options.entropy ||

account.options.entropy.type !== KeyringAccountEntropyTypeOption.Mnemonic

) {

console.warn(

"! Found an HD account with invalid entropy options: account won't be associated to its wallet.",

);

Fix in Cursor • Fix in Web

Was this report helpful? Give feedback by reacting with 👍 or 👎

Doing this defensive check sounds a bit too much to me given that Account extends KeyringAccount and options should always be defined.

cryptodev-2s

LGTM!

mcmire

Looks great! Just some minor comments.

mcmire · 2025-07-18T17:02:17Z

packages/multichain-account-service/src/MultichainAccountService.ts

+    // TODO: For now, we to every `KeyringController` state change to detect when
+    // new entropy sources/SRPs are being added. Having a dedicated event when
+    // new keyrings are added would make this more efficient.
+    this.#messenger.subscribe('KeyringController:stateChange', (state) => {
+      this.#setMultichainAccountWallets(state.keyrings);
+    });


You mention having a dedicated event, but there's already another way to do this that's more efficient.

The subscribe method takes a third argument, a function that works in spirit similar to input selectors in Redux (that is, they should be simple and only return a single property of the state object). This function should return the part of state that you are interested in observing.

What about this:

Suggested change

// TODO: For now, we to every `KeyringController` state change to detect when

// new entropy sources/SRPs are being added. Having a dedicated event when

// new keyrings are added would make this more efficient.

this.#messenger.subscribe('KeyringController:stateChange', (state) => {

this.#setMultichainAccountWallets(state.keyrings);

});

this.#messenger.subscribe(

'KeyringController:stateChange',

(keyringsFromState) => {

this.#setMultichainAccountWallets(keyringsFromState);

},

selectKeyringControllerKeyrings,

);

And then above the class MultichainAccountService line, you could add:

/** * Select keyrings from keyring controller state. * * @param state - The keyring controller state. * @returns The keyrings. */ function selectKeyringControllerKeyrings(state: KeyringControllerState) { return state.keyrings; }

Because the entire keyrings array is changed if a new keyring is added or removed, it will detect that state.keyrings is a new object reference and your subscription function will be called. And if no keyrings are added or removed, your subscription function will not be called.

Sounds like a good idea! I always forgot we have built-in selectors for controllers too! 🙈

Let me do this!

Fixed here: 7399ee3

packages/multichain-account-service/src/providers/BaseAccountProvider.test.ts

mcmire · 2025-07-18T17:04:15Z

packages/multichain-account-service/src/providers/EvmAccountProvider.ts

+  isAccountCompatible(account: InternalAccount): boolean {
+    return (
+      account.type === EthAccountType.Eoa &&
+      account.metadata.keyring.type === (KeyringTypes.hd as string)


Is this type assertion needed?

Suggested change

account.metadata.keyring.type === (KeyringTypes.hd as string)

account.metadata.keyring.type === KeyringTypes.hd

Bug: Function Fails When Options Undefined

The isBip44Account function throws a TypeError if account.options is undefined, as it directly accesses account.options.entropy. This prevents the intended behavior of returning false and logging a warning. Use optional chaining (account.options?.entropy) to safely access the property.

packages/multichain-account-service/src/providers/BaseAccountProvider.ts#L26-L29

core/packages/multichain-account-service/src/providers/BaseAccountProvider.ts

Lines 26 to 29 in 7399ee3

    
           ): account is Bip44Account<Account> { 
        
             if ( 
        
               !account.options.entropy || 
        
               account.options.entropy.type !== KeyringAccountEntropyTypeOption.Mnemonic

Fix in Cursor • Fix in Web

Was this report helpful? Give feedback by reacting with 👍 or 👎

cryptodev-2s

LGTM!

ccharly added 4 commits July 17, 2025 18:13

feat: add multichain-account-controller implementation

d0db324

refactor: remove write operations + use new keyring account options

23e91ae

refactor: remove unused test code

4588bad

chore: update README

7c23474

ccharly force-pushed the feat/multichain-accounts-controller-readonly branch from 8ffe4ea to 7c23474 Compare July 17, 2025 16:53

ccharly added 2 commits July 17, 2025 18:59

chore: update CODEOWNERS

8e81e6a

chore: fix deps

ed8e17c

ccharly commented Jul 17, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

ccharly added 2 commits July 18, 2025 10:50

Merge branch 'main' into feat/multichain-accounts-controller-readonly

c99dd89

chore: remove use of preview builds

0c7ec8a

ccharly added team-accounts no-changelog labels Jul 18, 2025

ccharly added 3 commits July 18, 2025 10:57

chore: bump eth-snap-keyring (was missing)

e29e344

chore: update CODEOWNERS

1edf2eb

chore: restore previous resolution

51f4404

ccharly force-pushed the feat/multichain-accounts-controller-readonly branch from 9755a66 to 51f4404 Compare July 18, 2025 09:12

chore: update teams.json

d9d8f5d

ccharly marked this pull request as ready for review July 18, 2025 10:05

ccharly requested a review from a team as a code owner July 18, 2025 10:05

shane-t reviewed Jul 18, 2025

View reviewed changes

cryptodev-2s reviewed Jul 18, 2025

View reviewed changes

chore: typo

4c9532d

Co-authored-by: cryptodev-2s <[email protected]>

This comment was marked as outdated.

Sign in to view

hmalik88 reviewed Jul 18, 2025

View reviewed changes

ccharly added 5 commits July 18, 2025 15:25

chore: add more comment

de60d19

chore: add missing jsdocs

d452337

feat: handle keyring state change for new entropy sources

0891e58

fix: fix jsdocs

7e3c5a8

test: fix error messages

349826f

chore: rename MultichainAccountController -> MultichainAccountService

9c0c6b6

ccharly dismissed shane-t’s stale review via 9c0c6b6 July 18, 2025 15:49

ccharly added 2 commits July 18, 2025 17:49

chore: cosmetic

f2c8ca5

chore: cosmetic

b26e629

This comment was marked as outdated.

Sign in to view

ccharly changed the title ~~feat: add multichain-account-controller (readonly)~~ feat: add multichain-account-service (readonly) Jul 18, 2025

cryptodev-2s previously approved these changes Jul 18, 2025

View reviewed changes

hmalik88 previously approved these changes Jul 18, 2025

View reviewed changes

mcmire reviewed Jul 18, 2025

View reviewed changes

test: remove unneeded beforeEach

d6dd7f0

Co-authored-by: Elliot Winkler <[email protected]>

ccharly dismissed stale reviews from hmalik88 and cryptodev-2s via d6dd7f0 July 18, 2025 21:33

This comment was marked as outdated.

Sign in to view

chore: export events + actions

6a1d472

Co-authored-by: Elliot Winkler <[email protected]>

This comment was marked as outdated.

Sign in to view

ccharly added 2 commits July 18, 2025 23:47

test: remove accounts.test.ts

18a6358

refactor: use event selector

7399ee3

cursor bot reviewed Jul 18, 2025

View reviewed changes

chore: update changelog

90a98a3

ccharly enabled auto-merge (squash) July 21, 2025 08:07

PatrykLucka approved these changes Jul 21, 2025

View reviewed changes

cryptodev-2s removed the no-changelog label Jul 21, 2025

cryptodev-2s approved these changes Jul 21, 2025

View reviewed changes

Merge branch 'main' into feat/multichain-accounts-controller-readonly

742486e

ccharly merged commit c3a0150 into main Jul 21, 2025
219 checks passed

ccharly deleted the feat/multichain-accounts-controller-readonly branch July 21, 2025 10:29

	account.metadata.keyring.type === (KeyringTypes.hd as string)
	account.metadata.keyring.type === KeyringTypes.hd

	): account is Bip44Account<Account> {
	if (
	!account.options.entropy \|\|
	account.options.entropy.type !== KeyringAccountEntropyTypeOption.Mnemonic

Uh oh!

feat: add multichain-account-service (readonly) #6141

feat: add multichain-account-service (readonly) #6141

Uh oh!

Conversation

ccharly commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

References

Changelog

Checklist

Uh oh!

socket-security bot commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cryptodev-2s left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cryptodev-2s commented Jul 18, 2025

Uh oh!

ccharly commented Jul 18, 2025

Uh oh!

cryptodev-2s commented Jul 18, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

ccharly commented Jul 18, 2025

Bug: Bip44Account Function Error Handling

Uh oh!

cryptodev-2s left a comment

Choose a reason for hiding this comment

Uh oh!

mcmire left a comment

Choose a reason for hiding this comment

Uh oh!

mcmire Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

feat: add `multichain-account-service` (readonly) #6141

feat: add `multichain-account-service` (readonly) #6141

ccharly commented Jul 17, 2025 •

edited

Loading

socket-security bot commented Jul 17, 2025 •

edited

Loading

cryptodev-2s left a comment •

edited

Loading

mcmire Jul 18, 2025 •

edited

Loading