fix(accounts): dynamically enable Money account keyrings and service#29502
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
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.
Reviewed by Cursor Bugbot for commit ee29ab2. Configure here.
| 'MoneyAccountController: error handling RemoteFeatureFlagController state change', | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Race condition in async flag handler can orphan money accounts
Low Severity
The async subscription callback has no guard against concurrent execution. If the flag toggles ON→OFF rapidly: (1) the first callback starts await controller.init(), (2) the second callback reads hasMoneyAccount as false (init not complete) and enters neither branch. When init() resolves, a money account exists but the flag is OFF — an inconsistent state that won't self-correct until another flag change event fires.
Reviewed by Cursor Bugbot for commit ee29ab2. Configure here.
There was a problem hiding this comment.
IMO that's super unlikely to happen, plus, restarting the app will, anyway, fix the state. WDYT @gantunesr?
There was a problem hiding this comment.
If the flag toggles ON→OFF rapidly
This is not a possible scenario in production. The flag has been designated as a "kill switch" therefore this is not a concern
There was a problem hiding this comment.
Yes, this is a non-reversible change
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection: The PR makes several interconnected changes to the MoneyAccountController and KeyringController initialization:
Tag Selection Rationale:
Performance Test Selection: |
|
Description
Keep the
MoneyKeyringbuilder registered in theKeyringControllerso that if the feature flag gets enabled dynamically, the controller and keyring will get created dynamically too!Changelog
CHANGELOG entry: N/A
Related issues
Fixes: TODO
Manual testing steps
Make sure to enable this in your
.js.env:Here's a patch to get some logs:
If you enabled those extra logs (with the patch above), toggling ON/OFF should give you something like this:
inittwice)Screenshots/Recordings
Before
After
Pre-merge author checklist
Performance checks (if applicable)
trace()for usage andaddTokenfor an exampleFor performance guidelines and tooling, see the Performance Guide.
Pre-merge reviewer checklist
Note
Medium Risk
Touches account/keyring initialization and wallet reset flows, so mistakes could create or wipe Money account state unexpectedly when feature flags change. Changes are scoped and covered by targeted unit tests, but still impact core account plumbing.
Overview
Money accounts are now managed dynamically based on remote feature-flag updates.
MoneyAccountControllersubscribes toRemoteFeatureFlagController:stateChangeduring init and willinit()when the flag turns on (only if the keyring is unlocked and no money accounts exist), orclearState()when the flag turns off (only if money accounts exist), with error logging on failures.Keyring handling is made resilient to flag timing. The
MoneyKeyringbuilder is now always registered inkeyringControllerInitso vault deserialization can recognize the type even if the Money feature flag is disabled at that moment.State clearing responsibilities are centralized.
Engine.resetStatenow clearsMoneyAccountControllerstate, whileAccountTreeInitService.clearStateno longer clears money accounts. Tests were updated/added to assert these behaviors and the new init messenger wiring (getMoneyAccountControllerInitMessenger).Reviewed by Cursor Bugbot for commit 9d427ea. Bugbot is set up for automated code reviews on this repo. Configure here.