Merged
Conversation
ccharly
force-pushed
the
cc/feat/money-account-controller
branch
2 times, most recently
from
e754aaa to
373edc9
Compare
Contributor
Author
|
@metamaskbot publish-preview |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
ccharly
force-pushed
the
cc/feat/money-account-controller
branch
2 times, most recently
from
a2a81b2 to
4615569
Compare
ccharly
force-pushed
the
cc/feat/money-account-controller
branch
from
4615569 to
9eb6b60
Compare
ccharly
commented
Apr 1, 2026
| languageName: node | ||
| linkType: hard | ||
|
|
||
| "@metamask/keyring-api@npm:^22.0.0": |
Contributor
Author
There was a problem hiding this comment.
We'll align this package later.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ccharly
commented
Apr 2, 2026
| KeyringTypes.money, | ||
| { | ||
| entropySource, | ||
| } as MoneyKeyringSerializedState, |
Contributor
Author
There was a problem hiding this comment.
Adding a type-constraint here so we know the data payload is well-shaped.
ccharly
commented
Apr 2, 2026
ccharly
commented
Apr 2, 2026
Comment on lines
+36
to
+40
| export type MoneyAccountControllerState = { | ||
| moneyAccounts: { | ||
| [id: MoneyAccount['id']]: MoneyAccount; | ||
| }; | ||
| }; |
Contributor
Author
There was a problem hiding this comment.
I followed a similar pattern than the internalAccounts.accounts mapping. We still have a unique ID for those accounts and having a mapping would allow us to have more than 1 MoneyAccount if we decide too (makes it also easier to combine "normal accounts" + "money accounts" IMO)
Contributor
Author
|
@metamaskbot publish-preview |
danroc
reviewed
Apr 2, 2026
packages/money-account-controller/src/MoneyAccountController.ts
Outdated
Show resolved
Hide resolved
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
danroc
reviewed
Apr 2, 2026
danroc
reviewed
Apr 2, 2026
| // Try to find an existing `MoneyKeyring` for this entropy source and get its address. | ||
| // If no such keyring exists, create one and add an account to get the address. | ||
| let address: Hex; | ||
| try { |
Contributor
There was a problem hiding this comment.
Personal opinion, but instead of doing a try/catch, I think it would be more natural to do a #getOrCreateKeyring then #getOrCreateAccount.
Contributor
Author
There was a problem hiding this comment.
Yep, as discussed, here's the new pattern:
packages/money-account-controller/src/MoneyAccountController.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| persist: true, | ||
| usedInUi: true, | ||
| }, | ||
| } satisfies StateMetadata<MoneyAccountControllerState>; |
There was a problem hiding this comment.
State persistence contradicts agreed-upon PR discussion
Medium Severity
The moneyAccounts metadata has persist: true, but the PR discussion between reviewers concluded that the state should not be persisted and instead be recreated inside init(). @ccharly said "We could start with un-persisted state for now" and @danroc confirmed "Indeed, it's probably better to recreate the state inside init instead of persisting it." With persist: true, stale money account data could survive across sessions even if the underlying keyring state has changed, and clearing/resetting becomes more fragile.
Contributor
Author
There was a problem hiding this comment.
So, for now, we'll keep things simple. Our init and clearState are hooked in our app lifecycle, which means we should never be able to end up with stale Money accounts.
Also, our primary focus is to have a Money account for our primary HD keyring for now (hence the init implementation here).
We can think of adding a resync to re-synchronize the Money account states with what we have in the associated keyrings. But that will come later.
danroc
approved these changes
Apr 2, 2026
Mrtenz
approved these changes
Apr 2, 2026
micaelae
pushed a commit
that referenced
this pull request
Apr 7, 2026
## Explanation Initial implementation of the `MoneyAccountController`. ## References 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](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Introduces new controller logic that creates and manages a new `MoneyKeyring` via `KeyringController`, including concurrency locking and state persistence; bugs here could affect keyring creation behavior and account derivation for Money accounts. > > **Overview** > Adds a new package, `@metamask/money-account-controller`, implementing `MoneyAccountController` to create and track **one Money account per entropy source** (defaulting to the primary HD keyring) and expose `createMoneyAccount`, `getMoneyAccount`, and `clearState` via the messenger. > > The controller integrates with `KeyringController` to find or create a `MoneyKeyring`, defensively ensures an address exists, and uses a mutex to prevent duplicate keyring creation under concurrent calls; comprehensive Jest tests and package build/docs configs are included. > > Repo wiring is updated to include the new package in `README.md`, root `tsconfig` references, `CODEOWNERS`, `teams.json`, and `yarn.lock` (including adding `@metamask/eth-money-keyring` and bumping `@metamask/eth-hd-keyring` resolution). > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 1beb776. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
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
Initial implementation of the
MoneyAccountController.References
N/A
Checklist
Note
Medium Risk
Introduces new controller logic that creates and manages a new
MoneyKeyringviaKeyringController, including concurrency locking and state persistence; bugs here could affect keyring creation behavior and account derivation for Money accounts.Overview
Adds a new package,
@metamask/money-account-controller, implementingMoneyAccountControllerto create and track one Money account per entropy source (defaulting to the primary HD keyring) and exposecreateMoneyAccount,getMoneyAccount, andclearStatevia the messenger.The controller integrates with
KeyringControllerto find or create aMoneyKeyring, defensively ensures an address exists, and uses a mutex to prevent duplicate keyring creation under concurrent calls; comprehensive Jest tests and package build/docs configs are included.Repo wiring is updated to include the new package in
README.md, roottsconfigreferences,CODEOWNERS,teams.json, andyarn.lock(including adding@metamask/eth-money-keyringand bumping@metamask/eth-hd-keyringresolution).Written by Cursor Bugbot for commit 1beb776. This will update automatically on new commits. Configure here.