feat: add money account keyring to keyring controller, and create cash account service#8204
Open
feat: add money account keyring to keyring controller, and create cash account service#8204
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Jwhiles
force-pushed
the
add-cash-keyring
branch
from
03c7d08 to
54bc644
Compare
Author
|
@metamaskbot publish-preview |
Jwhiles
force-pushed
the
add-cash-keyring
branch
from
54bc644 to
03c168a
Compare
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Jwhiles
force-pushed
the
add-cash-keyring
branch
from
03c168a to
0dac2dc
Compare
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Jwhiles
force-pushed
the
add-cash-keyring
branch
from
0dac2dc to
d86f255
Compare
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Jwhiles
changed the title
Jwhiles
force-pushed
the
add-cash-keyring
branch
from
f872b00 to
ad6c4e1
Compare
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Jwhiles
commented
Mar 17, 2026
|
|
||
| export type CashAccountServiceActions = CashAccountServiceMethodActions; | ||
|
|
||
| type AllowedActions = |
Author
There was a problem hiding this comment.
Possibly should export these to make it easier to instantiate this service
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
danroc
reviewed
Mar 18, 2026
| * @param entropySource - The metadata id of the HD keyring to derive from. | ||
| * @returns The metadata of the newly created Cash keyring. | ||
| */ | ||
| async createCashAccount(entropySource: string): Promise<KeyringMetadata> { |
Contributor
There was a problem hiding this comment.
Can you make this method idempotent and ensure that it does not add a new keyring if one already exists?
ccharly
reviewed
Mar 18, 2026
Comment on lines
+36
to
+37
| async ({ keyring }) => { | ||
| const hdKeyring = keyring as unknown as HdKeyring; |
Contributor
There was a problem hiding this comment.
Let's add a guard here, we should probably check that keyring.type is correct.
Also, HdKeyring already implements Keyring, so we shouldn't need the as unknown I think, just up-casting is enough (and the keyring.type check should """guarantee""" that we can upcast in a safe way).
Comment on lines
+43
to
+45
| return hdKeyring.mnemonic; | ||
| }, | ||
| )) as Uint8Array; |
Contributor
There was a problem hiding this comment.
We shouldn't need to cast here neither. We already checked for !hdKeyring.mnemonic so Uint8 | null would be inferred as Uint8 from now on?
Suggested change
| return hdKeyring.mnemonic; | |
| }, | |
| )) as Uint8Array; | |
| return hdKeyring.mnemonic; | |
| }, | |
| )); |
| /* eslint-disable @typescript-eslint/naming-convention */ | ||
| simple = 'Simple Key Pair', | ||
| hd = 'HD Key Tree', | ||
| cash = 'Cash Keyring', |
Contributor
There was a problem hiding this comment.
We should report this in the changelog
teams.json
Outdated
| "metamask/base-controller": "team-core-platform", | ||
| "metamask/base-data-service": "team-core-platform", | ||
| "metamask/build-utils": "team-core-platform", | ||
| "metamask/cash-account-service": "team-earn", |
Contributor
There was a problem hiding this comment.
We probably should be owner of this too?
Suggested change
| "metamask/cash-account-service": "team-earn", | |
| "metamask/cash-account-service": "team-accounts-framework,team-earn", |
Comment on lines
+25
to
+32
| /** | ||
| * Creates a Cash keyring derived from the HD keyring identified by | ||
| * the given entropy source, and returns the new keyring's metadata. | ||
| * | ||
| * @param entropySource - The metadata id of the HD keyring to derive from. | ||
| * @returns The metadata of the newly created Cash keyring. | ||
| */ | ||
| async createCashAccount(entropySource: string): Promise<KeyringMetadata> { |
Contributor
There was a problem hiding this comment.
So, I'm a bit lost with the naming here 😅
We use createCashAccount but this does not return a KeyringAccount...
From my understanding we should have those requirements:
- Always have 1 Cash keyring per HD keyring (1-to-1 binding)
- Always have 1 Cash account per Cash keyring
Maybe we should have 2 separate methods for this:
#getOrCreateCashKeyring(entropySource): Promise<CashKeyring>, this one has to be idempotent so we do not re-create the keyring everytimegetCashAccount(entropySource): Promise<KeyringAccount>, this would be the only public method of this service (this must also be idempotent).- I used
getsince cash accounts seems to be "implicitly" created for each HD keyrings we have, so semantically this looks more correct to me WDYT?
- I used
We could also listen for KeyringController:stateChange, but this one triggers quite a lot and might add a bit too much overhead (and we cannot use selector for this one since the state.keyrings reference is getting change on every vault update IIRC)
Contributor
There was a problem hiding this comment.
Also, do we really need the KeyringMetadata here? Do you need to reference the keyring ID somewhere else?
Member
There was a problem hiding this comment.
@ccharly We possibly don't want 1 cash account per HD keyring. The user only expects to see one account. I've added that to the FAQ here
It's not necessarily wrong to have one but the additional ones are not required.
We also don't necessarily want 1 cash account per cash account keyring - we want between zero and one. The account is created when it is needed.
I mention it because you propose a getCashAccount(entropySource), but we do want to expose a method for getting the canonical one.
Contributor
There was a problem hiding this comment.
Oh then, if that's only 1 cash only no matter how many SRPs you have, then we should definitely not expose the entropySource as a parameter IMO.
We should be using the default primary SRP under the hood (that would be the service's logic)!
Like we hide all the necessary logic to create the MoneyKeyring and create the MoneyAccount. The consume can just use service.getMoneyAccount() to get it, that sounds pretty neat and simple to me.
Author
There was a problem hiding this comment.
That does sound neat, but there could be something valuable about having to explicitly call createMoneyAccount the first time we want to access it? I wonder if there may be some restrictions about which users we can create these accounts for - and making the creation process very explicit might help us avoid compliance issues.
That said, I don't actually know of any strict requirements along those lines. I'm just hypothesising! Happy to change the naming if you feel strongly.
Member
There was a problem hiding this comment.
I think what @ccharly is saying makes sense and it's probably compatible with the stipulation @Jwhiles highlights.
It does make sense to separate account initialisation for those reasons - one reason is that some other services (Ramps, etc) might behave differently before the user has the account (and as @Jwhiles mentioned, cash accounts might be geo-restricted).
It could be possible to use the default SRP under the hood, AND have an explicit, idempotent call to create the account. We do need a getMoneyAccount() method which resolve falsily until then
Author
There was a problem hiding this comment.
So we are leaning towards two methods?
getMoneyAccount -> returns money account if one exists, else null/false/whatever
createMoneyAccount -> creates and returns money account if none exists else it either throws, returns null, or returns the account depending on what we decide
.github/CODEOWNERS
Outdated
|
|
||
| ## Earn Team | ||
| /packages/earn-controller @MetaMask/metamask-earn | ||
| /packages/cash-account-service @MetaMask/metamask-earn |
Contributor
There was a problem hiding this comment.
We should probably be owner of this too, so this must be moved in the "Joint team" section
|
|
||
| ### Added | ||
|
|
||
| - Initial release |
Contributor
There was a problem hiding this comment.
Suggested change
| - Initial release | |
| - Initial release ([#8204](https://github.com/MetaMask/core/pull/8204)) |
Author
|
@metamaskbot publish-preview |
Jwhiles
force-pushed
the
add-cash-keyring
branch
from
a175847 to
cd47082
Compare
Jwhiles
force-pushed
the
add-cash-keyring
branch
from
5902b98 to
fa0c1cb
Compare
| "test:watch": "NODE_OPTIONS=--experimental-vm-modules jest --watch" | ||
| }, | ||
| "dependencies": { | ||
| "@metamask/base-controller": "^9.0.0", |
There was a problem hiding this comment.
Unused @metamask/base-controller dependency in new package
Low Severity
@metamask/base-controller is listed as a production dependency and referenced in tsconfig.build.json, but no source file in the package imports from it. The service class does not extend BaseController (which is correct per the AGENTS.md guideline that stateless services should not be controllers). This dependency and its tsconfig reference appear to be unnecessary.
Additional Locations (1)
Jwhiles
force-pushed
the
add-cash-keyring
branch
from
1b134e3 to
355208d
Compare
Jwhiles
changed the title
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Jwhiles
force-pushed
the
add-cash-keyring
branch
from
bbf5917 to
04f063b
Compare
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
This PR adds the new cash account keyring, as a well as a new service which will be used to add accounts to that keyring.
This PR is dependant on this PR in the accounts repo being merged and published.
References
Checklist
Note
Medium Risk
Medium risk because it extends core keyring infrastructure (new
KeyringTypes.moneyand default keyring builder) and introduces a service that derives new keyrings from HD mnemonics, which could affect account/key management flows if misused.Overview
Adds support for Money accounts by introducing
KeyringTypes.moneyand registeringMoneyKeyring(from@metamask/eth-money-keyring) as a built-in default keyring inKeyringController.Introduces a new package,
@metamask/money-account-service, exposing messenger-callable methods tocreateMoneyAccount(derive a Money keyring from an HD keyring’s mnemonic/entropy source) andgetMoneyAccount(return existing Money keyring metadata), with full unit tests and build/test/docs config.Updates
accounts-controllerto displayKeyringTypes.moneyasMoney Accountand extends tests accordingly, and wires repo ownership/build plumbing (CODEOWNERS,teams.json, roottsconfig.build.json,yarn.lock).Written by Cursor Bugbot for commit 638782e. This will update automatically on new commits. Configure here.