feat(accounts-controller): add new typed options for InternalAccounts
#6147
+701
−285
Conversation
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
ccharly
force-pushed
the
feat/use-keyring-typed-options
branch
from
9d952f6 to
254e0a5
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
montelaidev
reviewed
Jul 22, 2025
Comment on lines
+157
to
+159
| // If for some reason, we cannot find this address, then the caller made a mistake | ||
| // and it did not use the proper keyring object. For now, we do not fail and just | ||
| // consider this account as "simple account". |
There was a problem hiding this comment.
undefined would be equivalent to a simple account? That doesn't seem correct.
There was a problem hiding this comment.
The other option would be to throw here. But that could be really risky and could brick wallets if not handled I guess 🤔
Also, this should NEVER happen, truly 😅 but I need to handle the error case.
Do you have anything else in mind for this case?
Comment on lines
582
to
584
| } else { | ||
| keyringTypes.set(keyringTypeName, 1); | ||
| keyringAccountIndexes.set(keyringTypeName, 1); | ||
| } |
There was a problem hiding this comment.
Don't we start at 0 index?
There was a problem hiding this comment.
Well, for "account naming" we do start at 1 (and I just re-used the previous logic, I just renamed the variables to make it more clear.
That being said, with the current logic, the keyringAccountIndexes will always be 1 and I think the if is not necessary actually, because:
const keyringAccountIndex = 0;
keyringAccountIndexes.set(keyringTypeName, keyringAccountIndex + 1);
// Would set `keyringAccountIndexes` to 1.
const keyringAccountIndex = 0;
if (keyringAccountIndex) {
keyringTypes.set(keyringTypeName, keyringAccountIndex + 1);
} else {
// This branch will get executed.
keyringTypes.set(keyringTypeName, 1);
// Would set `keyringAccountIndexes` to 1.
}
So they do seem similar.
Lastly, we mostly use those indexes to compute the account name, but I do believe we never end up using this one, cause we ALWAYS set a name when creating an account 🤔 So this fallback is probably never executed and probably buggy too 🙃
I'll try to fix it still.
There was a problem hiding this comment.
Maybe we can add a deprecation notice to the name property in the internal account. The account name will come from the account group.
There was a problem hiding this comment.
Maybe we can add a deprecation notice to the name property in the internal account. The account name will come from the account group.
Sounds like a good idea, but IMO we should do that once we truly introduce account group naming!
|
@metamaskbot publish-preview |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
Note: Simple private key accounts and hardware wallets do not have their accounts options automatically populated. It is not needed for state 2, and will be addressed in a subsequent PR. |
montelaidev
approved these changes
Jul 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Properly populate entropy options for EVM accounts and also add new typed options for every
InternalAccounts.Test PR:
References
N/A
Changelog
N/A
Checklist