Commit a6572a0
authored
fix(accounts): dynamically enable Money account keyrings and service (#29502)
## **Description**
Keep the `MoneyKeyring` builder registered in the `KeyringController` so
that if the feature flag gets enabled dynamically, the controller and
keyring will get created dynamically too!
- The money keyring state is never removed/cleared
- When the flag goes from ON -> OFF and there was a Money account, it
gets cleared
- When the flag goes from OFF -> ON and there was no Money account, it
gets created automatically
## **Changelog**
CHANGELOG entry: N/A
## **Related issues**
Fixes: TODO
## **Manual testing steps**
Make sure to enable this in your `.js.env`:
```env
DEBUG=metamask:money-account-controller
```
Here's a patch to get some logs:
```diff
diff --git a/app/core/Engine/controllers/money-account-controller-init.ts b/app/core/Engine/controllers/money-account-controller-init.ts
index c493253..b5d5f4b761 100644
--- a/app/core/Engine/controllers/money-account-controller-init.ts
+++ b/app/core/Engine/controllers/money-account-controller-init.ts
@@ -39,12 +39,14 @@ export const moneyAccountControllerInit: MessengerClientInitFunction<
const { isUnlocked } = initMessenger.call(
'KeyringController:getState',
);
+ console.log('testing: Initializing money account due to FF on');
// Check for the `KeyringController` to be unlocked, otherwise we won't be able
// to create the Money keyring if it doesn't exist yet!
if (isUnlocked) {
// This call is idempotent, so it is safe to call even if the
// controller is already initialized.
await controller.init();
+ console.log('testing: Clearing money account state due to FF off');
}
} else if (!isEnabled && hasMoneyAccount) {
// Clear state if we had a previous Money account and FF is off.
diff --git a/app/core/Engine/controllers/remote-feature-flag-controller-init.ts b/app/core/Engine/controllers/remote-feature-flag-controller-init.ts
index 2ab54cc..2ac6fb26d9 100644
--- a/app/core/Engine/controllers/remote-feature-flag-controller-init.ts
+++ b/app/core/Engine/controllers/remote-feature-flag-controller-init.ts
@@ -45,9 +45,7 @@ export const remoteFeatureFlagControllerInit: MessengerClientInitFunction<
distribution: getFeatureFlagAppDistribution(),
},
}),
- fetchInterval: __DEV__
- ? 1000
- : AppConstants.FEATURE_FLAGS_API.DEFAULT_FETCH_INTERVAL,
+ fetchInterval: 1000,
});
if (disabled) {
@@ -61,6 +59,15 @@ export const remoteFeatureFlagControllerInit: MessengerClientInitFunction<
Logger.log('Feature flags updated');
})
.catch((error) => Logger.log('Feature flags update failed: ', error));
+ setInterval(() =>
+ controller
+ .updateRemoteFeatureFlags()
+ .then(() => {
+ Logger.log('Feature flags updated (interval)');
+ })
+ .catch((error) => Logger.log('Feature flags update failed: ', error))
+ , 10 * 1000
+ );
}
return {
diff --git a/app/lib/Money/feature-flags.ts b/app/lib/Money/feature-flags.ts
index 736ed10..464497e8dc 100644
--- a/app/lib/Money/feature-flags.ts
+++ b/app/lib/Money/feature-flags.ts
@@ -16,6 +16,7 @@ export function isMoneyAccountEnabled(
const localFlag = process.env.MM_MONEY_ENABLE_MONEY_ACCOUNT === 'true';
const remoteFlag =
remoteFeatureFlags?.moneyEnableMoneyAccount as VersionGatedFeatureFlag;
+ console.log('testing: Remote Flag is:', remoteFlag, 'Local Flag is:', localFlag);
return validatedVersionGatedFeatureFlag(remoteFlag) ?? localFlag;
}
```
```gherkin
Feature: Money account feature flag handling (ON)
Scenario: flag is ON
Given the flag was OFF
When the flag gets updated
Then the Money account gets created automatically
Feature: Money account feature flag handling (OFF)
Scenario: flag is OFF
Given the flag was ON
When the flag gets updated
Then the Money account gets cleared automatically
```
If you enabled those extra logs (with the patch above), toggling ON/OFF
should give you something like this:
```log
$ yarn watch |& grep -E "metamask:money-account-controller|testing:"
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": true, "minimumVersion": "0.0.0"} Local Flag is: false
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": true, "minimumVersion": "0.0.0"} Local Flag is: false
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": true, "minimumVersion": "0.0.0"} Local Flag is: false
(NOBRIDGE) DEBUG metamask:money-account-controller Money keyring (entropy:01KQFS1XQNH0399NJM7EZEFWP7 - primary) account is: 0xad9d9f06da37139dd54fd48fda02cae244a590cb (b3961609-ef94-42ea-90fc-93149e185d56) +0ms
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": true, "minimumVersion": "0.0.0"} Local Flag is: false
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": false, "minimumVersion": "0.0.0"} Local Flag is: false
(NOBRIDGE) LOG testing: Clearing money account state due to FF off
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": true, "minimumVersion": "0.0.0"} Local Flag is: false
(NOBRIDGE) LOG testing: Initializing money account due to FF on
(NOBRIDGE) DEBUG metamask:money-account-controller Money keyring (entropy:01KQFS1XQNH0399NJM7EZEFWP7) account created: 0xad9d9f06da37139dd54fd48fda02cae244a590cb (b3961609-ef94-42ea-90fc-93149e185d56) +26s
(NOBRIDGE) DEBUG metamask:money-account-controller Money keyring (entropy:01KQFS1XQNH0399NJM7EZEFWP7 - primary) account is: 0xad9d9f06da37139dd54fd48fda02cae244a590cb (b3961609-ef94-42ea-90fc-93149e185d56) +0ms
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": true, "minimumVersion": "0.0.0"} Local Flag is: false
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": true, "minimumVersion": "0.0.0"} Local Flag is: false
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": false, "minimumVersion": "0.0.0"} Local Flag is: false
(NOBRIDGE) LOG testing: Clearing money account state due to FF off
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": false, "minimumVersion": "0.0.0"} Local Flag is: false
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": false, "minimumVersion": "0.0.0"} Local Flag is: false
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": true, "minimumVersion": "0.0.0"} Local Flag is: false
(NOBRIDGE) LOG testing: Initializing money account due to FF on
(NOBRIDGE) DEBUG metamask:money-account-controller Money keyring (entropy:01KQFS1XQNH0399NJM7EZEFWP7) account created: 0xad9d9f06da37139dd54fd48fda02cae244a590cb (b3961609-ef94-42ea-90fc-93149e185d56) +1m
(NOBRIDGE) DEBUG metamask:money-account-controller Money keyring (entropy:01KQFS1XQNH0399NJM7EZEFWP7 - primary) account is: 0xad9d9f06da37139dd54fd48fda02cae244a590cb (b3961609-ef94-42ea-90fc-93149e185d56) +0ms
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": true, "minimumVersion": "0.0.0"} Local Flag is: false
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": true, "minimumVersion": "0.0.0"} Local Flag is: false
(NOBRIDGE) LOG testing: Remote Flag is: {"enabled": true, "minimumVersion": "0.0.0"} Local Flag is: false
```
- We only re-init if there was no Money account AND the flag is ON
- We only clear if there was a Money account AND the flag is OFF
- We only do those steps once (not clearing twice, not calling `init`
twice)
## **Screenshots/Recordings**
### **Before**
### **After**
## **Pre-merge author checklist**
- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I've included tests if applicable
- [ ] I've documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I've applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
#### Performance checks (if applicable)
- [ ] I've tested on Android
- Ideally on a mid-range device; emulator is acceptable
- [ ] I've tested with a power user scenario
- Use these [power-user
SRPs](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/edit-v2/401401446401?draftShareId=9d77e1e1-4bdc-4be1-9ebb-ccd916988d93)
to import wallets with many accounts and tokens
- [ ] I've instrumented key operations with Sentry traces for production
performance metrics
- See [`trace()`](/app/util/trace.ts) for usage and
[`addToken`](/app/components/Views/AddAsset/components/AddCustomToken/AddCustomToken.tsx#L274)
for an example
For performance guidelines and tooling, see the [Performance
Guide](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/400085549067/Performance+Guide+for+Engineers).
## **Pre-merge reviewer checklist**
- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
<!-- CURSOR_SUMMARY -->
---
> [!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.** `MoneyAccountController` subscribes to
`RemoteFeatureFlagController:stateChange` during init and will `init()`
when the flag turns on (only if the keyring is unlocked and no money
accounts exist), or `clearState()` when the flag turns off (only if
money accounts exist), with error logging on failures.
>
> **Keyring handling is made resilient to flag timing.** The
`MoneyKeyring` builder is now always registered in
`keyringControllerInit` so vault deserialization can recognize the type
even if the Money feature flag is disabled at that moment.
>
> **State clearing responsibilities are centralized.**
`Engine.resetState` now clears `MoneyAccountController` state, while
`AccountTreeInitService.clearState` no longer clears money accounts.
Tests were updated/added to assert these behaviors and the new init
messenger wiring (`getMoneyAccountControllerInitMessenger`).
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
9d427ea. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->1 parent eedc67b commit a6572a0
10 files changed
Lines changed: 323 additions & 87 deletions
File tree
- app
- core/Engine
- controllers
- keyring-controller
- messengers
- multichain-accounts/AccountTreeInitService
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1368 | 1368 | | |
1369 | 1369 | | |
1370 | 1370 | | |
| 1371 | + | |
| 1372 | + | |
| 1373 | + | |
| 1374 | + | |
| 1375 | + | |
| 1376 | + | |
| 1377 | + | |
| 1378 | + | |
| 1379 | + | |
| 1380 | + | |
| 1381 | + | |
| 1382 | + | |
| 1383 | + | |
1371 | 1384 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1159 | 1159 | | |
1160 | 1160 | | |
1161 | 1161 | | |
| 1162 | + | |
1162 | 1163 | | |
1163 | 1164 | | |
1164 | 1165 | | |
| |||
1189 | 1190 | | |
1190 | 1191 | | |
1191 | 1192 | | |
| 1193 | + | |
| 1194 | + | |
| 1195 | + | |
1192 | 1196 | | |
1193 | 1197 | | |
1194 | 1198 | | |
| |||
Lines changed: 1 addition & 24 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | | - | |
3 | | - | |
4 | | - | |
5 | | - | |
6 | | - | |
7 | | - | |
8 | | - | |
9 | | - | |
10 | 2 | | |
11 | 3 | | |
12 | 4 | | |
| |||
72 | 64 | | |
73 | 65 | | |
74 | 66 | | |
75 | | - | |
76 | 67 | | |
77 | 68 | | |
78 | 69 | | |
| |||
108 | 99 | | |
109 | 100 | | |
110 | 101 | | |
111 | | - | |
112 | | - | |
113 | | - | |
| 102 | + | |
114 | 103 | | |
115 | 104 | | |
116 | 105 | | |
117 | 106 | | |
118 | 107 | | |
119 | | - | |
120 | | - | |
121 | | - | |
122 | | - | |
123 | | - | |
124 | | - | |
125 | | - | |
126 | | - | |
127 | | - | |
128 | | - | |
129 | | - | |
130 | | - | |
131 | 108 | | |
132 | 109 | | |
133 | 110 | | |
| |||
Lines changed: 27 additions & 33 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | | - | |
3 | 2 | | |
4 | 3 | | |
5 | 4 | | |
| |||
44 | 43 | | |
45 | 44 | | |
46 | 45 | | |
47 | | - | |
48 | | - | |
49 | | - | |
50 | | - | |
51 | 46 | | |
52 | 47 | | |
53 | 48 | | |
| |||
81 | 76 | | |
82 | 77 | | |
83 | 78 | | |
84 | | - | |
85 | | - | |
86 | | - | |
87 | | - | |
88 | | - | |
89 | | - | |
90 | | - | |
91 | | - | |
92 | | - | |
93 | | - | |
94 | | - | |
95 | | - | |
96 | | - | |
97 | | - | |
98 | | - | |
99 | | - | |
100 | | - | |
101 | | - | |
102 | | - | |
103 | | - | |
104 | | - | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
105 | 100 | | |
106 | | - | |
107 | | - | |
108 | | - | |
109 | | - | |
110 | | - | |
111 | | - | |
112 | | - | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
113 | 107 | | |
114 | 108 | | |
115 | 109 | | |
| |||
Lines changed: 182 additions & 11 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | | - | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
4 | 8 | | |
5 | 9 | | |
6 | 10 | | |
7 | 11 | | |
8 | 12 | | |
9 | 13 | | |
10 | 14 | | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
11 | 18 | | |
12 | 19 | | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
13 | 25 | | |
14 | | - | |
15 | | - | |
16 | | - | |
17 | | - | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
18 | 30 | | |
19 | | - | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
20 | 46 | | |
21 | | - | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
22 | 54 | | |
23 | 55 | | |
24 | | - | |
25 | | - | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
26 | 79 | | |
27 | 80 | | |
28 | 81 | | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
29 | 86 | | |
30 | | - | |
| 87 | + | |
| 88 | + | |
31 | 89 | | |
32 | 90 | | |
33 | 91 | | |
34 | 92 | | |
35 | | - | |
| 93 | + | |
| 94 | + | |
36 | 95 | | |
37 | 96 | | |
38 | 97 | | |
39 | 98 | | |
40 | 99 | | |
41 | 100 | | |
42 | 101 | | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
43 | 214 | | |
0 commit comments