-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Release/170.0.0 #4498
Release/170.0.0 #4498
Changes from 8 commits
f4e1b96
05b1337
cb09fd8
07256ab
1fd0e72
c37dda1
abb8d47
cb744f7
a79251c
32de691
283f758
3986e36
502f49c
644f74e
9860c57
78a8f58
247271c
6cb3dde
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
## [Unreleased] | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
## [17.2.0] | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
### Added | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
- Add `accountAdded` and `accountRemoved` events ([#4496](https://github.com/MetaMask/core/pull/4496)) | ||||||||||||||||||||||||||||||||||||||||||||||||
- Export `AccountsControllerListMultichainAccounts`,`AccountsControllerGetSelectedMultichainAccount`,`AccountsControllerGetNextAvailableAccountName}Action` actions ([#4497](https://github.com/MetaMask/core/pull/4497)) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
### Fixed | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
- Use `listMultichainAccounts` for non-EVM specific methods ([#4494](https://github.com/MetaMask/core/pull/4494)) | ||||||||||||||||||||||||||||||||||||||||||||||||
- Set `lastSelected` for initial account ([#4494](https://github.com/MetaMask/core/pull/4494)) | ||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the Fixed entries could be more user-friendly, and less reliant on knowledge of internal variables, but I'm lacking the understanding to come up with better phrasing. @montelaidev could you maybe provide some context on the user-facing changes here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mcmire Should we see the addition of new internal actions/events as a breaking change? My initial reasoning was that the added actions/events now need to be present in the generic arguments of the non-restricted controller-messenger, but I'm not sure about this since unlike
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MajorLift That's the way I've thought about it too: the reason it's a breaking change is that consumers now needs to allowlist that action or event when initializing the messenger, otherwise an error will be thrown when attempting to call or subscribe to that action or event. The fact that the messenger doesn't throw immediately if a used action or event is omitted from the allowlist is unfortunate, but there will be an error eventually. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the fixed entries, they are only internal changes that fixes a bug. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new events are internal AccountsController events though. It would be a breaking change if they were external actions/events that the AccountsController require. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For restricted messengers, only allowlists for external actions/events need to be supplied, but for unrestricted controller-messengers, consumers still define them by enumerating all actions/events e.g. new ControllerMessenger<
| AccountsControllerActions
| NetworkControllerActions
| NetworkControllerAllowedActions,
...,
>(); The part I'm confused about is whether to call this non-breaking because users have access to the union types that fully enumerate internal actions/events, or to call this breaking because users could choose to use individual action/event type exports, in which case they'd need to keep track of additions to the internal actions/events e.g. new ControllerMessenger<
// | TokensControllerGetStateAction
| TokensControllerAddDetectedTokensAction
| TokensControllerExampleNewAction,
...,
>();
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, good point. I am inclined in this case not to treat it as a breaking change. My reasoning is that even if a consumer left out There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm I was going to argue that adding these new internal actions/events represents additional requirements for the However, I've found that restricted messengers with incomplete or even empty internal actions/events lists are being accepted by our controller constructors, as long as they have compatible allowlists for external actions/events. I'll have to create a ticket for addressing this. I think it's accurate to say, at least for now, that this wouldn't be a breaking change for consumers. Once the controller has been instantiated, I agree that it makes sense to only look at whether the controller can publish the new events, and have the controller be agnostic about whether any given messenger (other than its own messagingSystem) subscribes to those events. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I put together a playground repro for this. Looks like constructor controllers do raise a type error if they're passed a messenger that was defined with an incomplete list of internal actions/events, but fail to raise that error if either the |
||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
## [17.1.1] | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
### Fixed | ||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -233,7 +245,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
- Initial release ([#1637](https://github.com/MetaMask/core/pull/1637)) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/accounts-controller@17.1.1...HEAD | ||||||||||||||||||||||||||||||||||||||||||||||||
[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/accounts-controller@17.2.0...HEAD | ||||||||||||||||||||||||||||||||||||||||||||||||
[17.2.0]: https://github.com/MetaMask/core/compare/@metamask/accounts-controller@17.1.1...@metamask/accounts-controller@17.2.0 | ||||||||||||||||||||||||||||||||||||||||||||||||
[17.1.1]: https://github.com/MetaMask/core/compare/@metamask/accounts-controller@17.1.0...@metamask/accounts-controller@17.1.1 | ||||||||||||||||||||||||||||||||||||||||||||||||
[17.1.0]: https://github.com/MetaMask/core/compare/@metamask/accounts-controller@17.0.0...@metamask/accounts-controller@17.1.0 | ||||||||||||||||||||||||||||||||||||||||||||||||
[17.0.0]: https://github.com/MetaMask/core/compare/@metamask/accounts-controller@16.0.0...@metamask/accounts-controller@17.0.0 | ||||||||||||||||||||||||||||||||||||||||||||||||
|
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.
Hi @montelaidev, any chance you could release the gas-fee-controller? The fix that I need released is #4446.