-
-
Notifications
You must be signed in to change notification settings - Fork 250
fix(accounts-controller): workaround compiler error for recursive type #6432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
ccharly
merged 5 commits into
main
from
fix/accounts-controller-relax-compiler-error-for-recursive-type
Sep 2, 2025
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
c145796
fix(accounts-controller): try to workaround compiler error for recurs…
ccharly ce34f63
refactor: remove old type casts
ccharly 8451a69
refactor: remove even more type cast
ccharly ddfdb19
refactor: add missing exportable option
ccharly 2ee6e81
Merge branch 'main' into fix/accounts-controller-relax-compiler-error…
ccharly File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import type { KeyringAccountEntropyOptions } from '@metamask/keyring-api'; | ||
import type { InternalAccount } from '@metamask/keyring-internal-api'; | ||
|
||
import type { AccountsControllerState } from './AccountsController'; | ||
|
||
/** | ||
* Type constraint to ensure a type is compatible with {@link AccountsControllerState}. | ||
* If the constraint is not matching, this type will resolve to `never` and thus, fails | ||
* to compile. | ||
*/ | ||
type IsAccountControllerState<Type extends AccountsControllerState> = Type; | ||
|
||
/** | ||
* A type compatible with {@link InternalAccount} which removes any use of recursive-type. | ||
*/ | ||
export type StrictInternalAccount = Omit<InternalAccount, 'options'> & { | ||
// Use stricter options, which are relying on `Json` (which sometimes | ||
// cause compiler errors because of instanciation "too deep". | ||
// In anyway, we should rarely have to use those "untyped" options. | ||
options: { | ||
entropy?: KeyringAccountEntropyOptions; | ||
exportable?: boolean; | ||
}; | ||
}; | ||
|
||
/** | ||
* A type compatible with {@link AccountControllerState} which can be used to | ||
* avoid recursive-type issue with `internalAccounts`. | ||
*/ | ||
export type AccountsControllerStrictState = IsAccountControllerState<{ | ||
internalAccounts: { | ||
accounts: Record<InternalAccount['id'], StrictInternalAccount>; | ||
selectedAccount: InternalAccount['id']; | ||
}; | ||
}>; |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Incompatible Account Types Cause Assignment Issues
The
StrictInternalAccount
type'soptions
property is overly restrictive, only allowingentropy
andexportable
. However,InternalAccount
objects, which are still used at runtime and in backups, contain additionaloptions
properties likeentropySource
,derivationPath
, andgroupIndex
. This type incompatibility causes issues when assigningInternalAccount
objects toStrictInternalAccount
properties, notably in the#update
andloadBackup
methods.Additional Locations (3)
packages/accounts-controller/src/AccountsController.ts#L605-L613
packages/accounts-controller/src/AccountsController.ts#L482-L484
packages/accounts-controller/src/AccountsController.ts#L526-L527
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true, but that's fine. We have no real use of those options, also, we can still update this type if we need too. As long as we don't use any recursive-type.
I'd just ignore this for now. Again, we're just narrowing a type, but we're not doing any runtime change at all, so all in all, this will behave the same.
Lastly, all those
#update
/update
calls are done internally, so if we need to wider the type, it won't bleed out on the public interface of this controller. So we're good on that front too.