Skip to content

Conversation

@ccharly
Copy link
Contributor

@ccharly ccharly commented Sep 11, 2025

Explanation

Bumping accounts dependencies and adapting new account-api and keyring-api breaking changes.

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, highlighting breaking changes as necessary
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@ccharly ccharly force-pushed the chore/bump-multichain-accounts-deps branch from 088c08b to 2b0b3fc Compare September 11, 2025 09:34
@socket-security
Copy link

socket-security bot commented Sep 11, 2025

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​metamask/​keyring-internal-api@​9.0.01001007198100
Added@​metamask/​account-api@​0.12.01001007497100
Added@​metamask/​keyring-api@​21.0.01001001009950
Added@​metamask/​eth-snap-keyring@​17.1.099100929950

View full report

@ccharly ccharly marked this pull request as ready for review September 11, 2025 09:52
@ccharly ccharly requested review from a team as code owners September 11, 2025 09:52
await this.alignAccounts();

return discoveredAccounts;
return providerContexts.flatMap((context) => context.accounts);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now only return discovered accounts. If we need to extract metrics from this, we can still rely on the account.type to know if it's a Solana/Bitcoin/EVM accounts that got discovered.

* Align all multichain account groups.
*/
async alignGroups(): Promise<void> {
async alignAccounts(): Promise<void> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to follow the <operation>Accounts pattern. Also, this function does re-align accounts (create missing one if needed)

But it WILL NOT create missing groups if there's any.

Thus, it's more accounts related.

* @returns The discovered accounts for each provider.
*/
async discoverAndCreateAccounts(): Promise<AccountDiscoveryMetrics> {
async discoverAccounts(): Promise<Account[]> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Just following the <operation>Accounts pattern.

Also the return type got changed, see this for more details.

},
scopes: [BtcScope.Testnet],
options: {},
methods: [BtcMethod.SendBitcoin],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method no longer exists. So we just use all methods from the enum now.

montelaidev
montelaidev previously approved these changes Sep 11, 2025
Copy link
Contributor

@montelaidev montelaidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Note: this should be breaking for the btc snap because of the keyring-api change.

@ccharly
Copy link
Contributor Author

ccharly commented Sep 11, 2025

LGTM. Note: this should be breaking for the btc snap because of the keyring-api change.

@montelaidev actually, I believe it's not breaking for the controller themselves. Most of them only depends on KeyringAccount or InternalAccount which have generic shapes.

However, we strongly-enforce account shapes at runtime in the Snap keyring:

And since the enum is only used for tests and not part of any public methods/types, none of this should be considered breaking IMO.

mathieuartu
mathieuartu previously approved these changes Sep 11, 2025
fabiobozzo
fabiobozzo previously approved these changes Sep 11, 2025
montelaidev
montelaidev previously approved these changes Sep 11, 2025
@ccharly ccharly enabled auto-merge (squash) September 11, 2025 12:22
montelaidev
montelaidev previously approved these changes Sep 11, 2025
},
"peerDependencies": {
"@metamask/account-api": "^0.9.0",
"@metamask/account-api": "^0.12.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: could we add a changelog for this peer dependency bump ?


### Changed

- **BREAKING:** Rename `MultichainAccountWallet.discoverAndCreateAccounts` to `MultichainAccountWallet.discoverAccounts` ([#6560](https://github.com/MetaMask/core/pull/6560))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: as AccountProviderWrapper, BaseBip44AccountProvider and SnapAccountProvider are also exported at the index level. Should we add a changelog entry for the same renaming ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes total sense too yes! I'll add those too. Thanks!

Copy link
Contributor

@cryptodev-2s cryptodev-2s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ccharly ccharly merged commit 63d5826 into main Sep 11, 2025
239 checks passed
@ccharly ccharly deleted the chore/bump-multichain-accounts-deps branch September 11, 2025 13:06
ccharly added a commit that referenced this pull request Sep 11, 2025
…utex for concurrent mutable operations (#6527)

## Explanation

Wallet operations are not behind a mutex. This ensure that only one
mutable operation can be executed at a time.

Also publish new event, so we know in which state a wallet is.

> [!NOTE]
> A follow-up PR will be using those events to add a new status in the
`account-tree-controller`.

Depends on this PR to be merged first:
- [ ] #6560

## References

N/A

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've communicated my changes to consumers by [updating changelogs
for packages I've
changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs),
highlighting breaking changes as necessary
- [ ] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes
mathieuartu added a commit that referenced this pull request Sep 29, 2025
## Explanation

Release for `@metamask/profile-sync-controller` &
`@metamask/multichain-account-service`.

```md
## [25.1.0]

### Changed

- Use deferred promises for encryption/decryption KDF operations ([#6736](#6736))
  - That will prevent duplicate KDF operations from being computed if one with the same options is already in progress.
  - For operations that already completed, we use the already existing cache.
- Bump `@metamask/utils` from `^11.8.0` to `^11.8.1` ([#6708](#6708))
- Bump `@metamask/keyring-api` from `^20.1.0` to `^21.0.0` ([#6560](#6560))
- Bump `@metamask/keyring-internal-api` from `^8.1.0` to `^9.0.0` ([#6560](#6560))
- Strip `srpSessionData.token.accessToken` from state logs ([#6553](#6553))
  - We haven't started using the `includeInStateLogs` metadata yet in clients, so this will have no functional impact. This change brings this metadata into alignment with the hard-coded state log generation performed by clients.today.
- Add dependency on `@metamask/utils` ([#6553](#6553))
- Bump `@metamask/base-controller` from `^8.3.0` to `^8.4.0` ([#6632](#6632))
```

## References

<!--
Are there any issues that this pull request is tied to?
Are there other links that reviewers should consult to understand these
changes better?
Are there client or consumer pull requests to adopt any breaking
changes?

For example:

* Fixes #12345
* Related to #67890
-->

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've communicated my changes to consumers by [updating changelogs
for packages I've
changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs),
highlighting breaking changes as necessary
- [ ] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes


<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> Releases core 589.0.0, publishing @metamask/profile-sync-controller
25.1.0 and @metamask/multichain-account-service 1.3.0, and bumps
dependent packages and changelogs.
> 
> - **Release**: `@metamask/core-monorepo` to `589.0.0`.
> - **Packages**:
>   - `@metamask/profile-sync-controller@25.1.0`
> - Use deferred promises for KDF operations; update deps and strip
`srpSessionData.token.accessToken` from state logs.
>   - `@metamask/multichain-account-service@1.3.0`
>     - Add `{Btc, Trx}AccountProvider`; update compare links.
> - **Deps Bumped**:
> - Update references to `@metamask/profile-sync-controller` to
`^25.1.0` in `account-tree-controller`,
`notification-services-controller`, `subscription-controller`, and
lockfile.
> - Update references to `@metamask/multichain-account-service` to
`^1.3.0` in `assets-controllers`, `account-tree-controller`, and
lockfile.
> - **Changelogs**:
> - Add Unreleased note for `assets-controllers` reflecting
`multichain-account-service` bump.
>   - Add `1.3.0` section and links in `multichain-account-service`.
>   - Add `25.1.0` section and links in `profile-sync-controller`.
> 
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
88351ca. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

---------

Co-authored-by: Antonio Regadas <antonio.regadas@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants