feat: add cash account keyring to keyring controller, and create cash account service#8204
Open
feat: add cash 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
|
|
||
| ## 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)) |
|
|
||
| if (existingCashMetadata) { | ||
| return existingCashMetadata; | ||
| } |
There was a problem hiding this comment.
Mnemonic fetched before existing keyring check breaks idempotency
Medium Severity
createCashAccount always retrieves the mnemonic from the HD keyring (lines 33–51) before checking whether a cash keyring already exists (lines 53–66). If the HD keyring identified by entropySource is missing or invalid, the method throws an error even when a valid cash keyring already exists and could simply be returned. Moving the existing-cash-keyring check before the mnemonic retrieval would make the method properly idempotent and avoid unnecessary work and lock acquisition on the KeyringController.
Author
|
@metamaskbot publish-preview |
Jwhiles
force-pushed
the
add-cash-keyring
branch
from
a175847 to
cd47082
Compare
| 'KeyringController:addNewKeyring', | ||
| KeyringTypes.cash, | ||
| { mnemonic }, | ||
| ); |
There was a problem hiding this comment.
Race condition allows duplicate cash keyring creation
Medium Severity
createCashAccount has a TOCTOU (time-of-check-time-of-use) race condition. The existence check via withKeyring and the creation via addNewKeyring are separate mutex-guarded calls with the lock released in between. Two concurrent calls can both pass the existence check and each create a separate cash keyring.
Author
There was a problem hiding this comment.
I added the check in response to your comment @danroc of
Can you make this method idempotent and ensure that it does not add a new keyring if one already exists?
I think this race condition is legit, and 'm struggling to see how to work around it without having a 'transaction' equivalent thing.
However, the money keyring only allows for a single account to be created anyway. So this protection is a duplicate. I wonder if we actually need it.
Jwhiles
force-pushed
the
add-cash-keyring
branch
from
5902b98 to
fa0c1cb
Compare
Jwhiles
force-pushed
the
add-cash-keyring
branch
from
fa0c1cb to
c1b78f8
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| return accounts.length > 0 ? metadata : null; | ||
| }, | ||
| ) | ||
| .catch(() => null)) as KeyringMetadata | null; |
There was a problem hiding this comment.
Blanket catch swallows errors beyond keyring-not-found
Low Severity
The .catch(() => null) on the withKeyring call silently swallows every possible error — not just the expected "keyring not found" case. If a cash keyring exists but keyring.getAccounts() throws due to corruption or an internal error, the failure is hidden and the code falls through to create a duplicate keyring instead of surfacing the real problem. Catching only the specific KeyringNotFound error would be safer.
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
High Risk
High risk because it changes keyring management by adding a new built-in keyring type and derives new key material from the HD keyring mnemonic via messenger calls. It also introduces new preview keyring dependencies, so regressions could impact account/key handling.
Overview
Adds Cash account support end-to-end:
KeyringControllernow includesKeyringTypes.cashand registersCashKeyring(from@metamask-previews/eth-cash-keyring) as a default built-in keyring.Introduces a new package,
@metamask/cash-account-service, exposingcreateCashAccountvia the messenger to derive a Cash keyring from an HD keyring mnemonic (or return existing Cash keyring metadata if one already has accounts), with accompanying tests and build/config scaffolding.Updates
accounts-controllerto displayKeyringTypes.cashasCash Account, expands tests to cover the new keyring type, and wires repo metadata (CODEOWNERS,teams.json, roottsconfig.build.json,yarn.lock) for the new package/dependencies.Written by Cursor Bugbot for commit c1b78f8. This will update automatically on new commits. Configure here.