Skip to content
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

Merged
merged 18 commits into from
Jul 3, 2024
Merged

Release/170.0.0 #4498

merged 18 commits into from
Jul 3, 2024

Conversation

montelaidev
Copy link
Contributor

@montelaidev montelaidev commented Jul 3, 2024

Explanation

Create release for:

  • @metamask/accounts-controller to ^17.2.0
  • @metamask/gas-fee-controller to ^18.0.0
  • @metamask/transaction-controller to ^34.0.0
  • @metamask/user-operation-controller to ^13.0.0

References

Changelog

@metamask/accounts-controller

  • ADDED: accountAdded and accountRemoved events (#4496)
  • ADDED: Export AccountsControllerListMultichainAccounts,AccountsControllerGetSelectedMultichainAccount,AccountsControllerGetNextAvailableAccountName}Action actions (#4497)
  • FIXED: Use listMultichainAccounts for non-EVM specific methods. (#4494)
  • FIXED: Set lastSelected for initial account (#4494)

@metamask/gas-fee-controller

  • BREAKING: Added back the constructor options legacyAPIEndpoint and EIP1559APIEndpoint. These URLs are no longer hardcoded within the controller.
  • BREAKING: Removed the infuraAPIKey. This was used to construct and send the Authorization header for Infura gas API requests. (#4446)
  • CHANGED: Changing providerConfig to selectedNetworkClientId (#4356)

@metamask/user-operation-controller

  • BREAKING: Bump dependency and peer dependency @metamask/gas-fee-controller to ^18.0.0 (#4498)
  • BREAKING: Bump dependency and peer dependency @metamask/transaction-controller to ^34.0.0 (#4498)

@metamask/transaction-controller

  • BREAKING: Bump dependency and peer dependency @metamask/gas-fee-controller to ^18.0.0 (#4498)

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 highlighted breaking changes using the "BREAKING" category above as appropriate

@montelaidev montelaidev requested a review from a team as a code owner July 3, 2024 13:45
package.json Outdated
Copy link
Contributor

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.

dbrans
dbrans previously approved these changes Jul 3, 2024
packages/gas-fee-controller/CHANGELOG.md Outdated Show resolved Hide resolved
packages/gas-fee-controller/CHANGELOG.md Outdated Show resolved Hide resolved
montelaidev and others added 2 commits July 4, 2024 00:35
Co-authored-by: Jongsun Suh <jongsun.suh@icloud.com>
dbrans
dbrans previously approved these changes Jul 3, 2024
Copy link
Contributor

@dbrans dbrans left a comment

Choose a reason for hiding this comment

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

Thank you @montelaidev!

@dbrans
Copy link
Contributor

dbrans commented Jul 3, 2024

@metamaskbot publish-preview

Comment on lines 10 to 20
## [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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## [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))
## [18.0.0]
### Added
- **BREAKING:** `AccountsControllerMessenger` must allow the new internal events `AccountsController:accountAdded` and `AccountsController:accountRemoved`. ([#4496](https://github.com/MetaMask/core/pull/4496))
- Add and export corresponding event types `AccountsControllerAccountAddedEvent`, `AccountsControllerAccountRemovedEvent`.
- Export action types `AccountsControllerListMultichainAccounts`,`AccountsControllerGetSelectedMultichainAccount`,`AccountsControllerGetNextAvailableAccountName}Action`. ([#4497](https://github.com/MetaMask/core/pull/4497))
### Fixed
- Use `listMultichainAccounts` instead of `listAccounts` for non-EVM specific multichain methods. ([#4494](https://github.com/MetaMask/core/pull/4494))
- For initial account, emit `selectedAccountChange ` and update `lastSelected`. ([#4494](https://github.com/MetaMask/core/pull/4494))

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

@MajorLift MajorLift Jul 3, 2024

Choose a reason for hiding this comment

The 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 Allowed{Actions,Events}, we do export ExampleController{Action,Event}s, enabling the consumer to define a controller-messenger without keeping up with changes to the internal actions/events.

Previous discussion: #4413 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@MajorLift MajorLift Jul 3, 2024

Choose a reason for hiding this comment

The 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,
  ...,
>();

(where tokens-controller only has the getState and addDetectedTokens actions)

Copy link
Contributor

@mcmire mcmire Jul 3, 2024

Choose a reason for hiding this comment

The 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 AccountsControllerAccountAddedEvent and AccountsControllerAccountRemovedEvent from the type parameter for the unrestricted controller messenger, the controller itself would still be able to emit the event. Whether the consumer is actually subscribing to the event or not is on the consumer, but I don't believe they'd be required to do that in order to upgrade. Does that reasoning make sense?

Copy link
Contributor

@MajorLift MajorLift Jul 3, 2024

Choose a reason for hiding this comment

The 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 messenger constructor option, and will cause type errors in downstream controller instantiations, especially where we're deriving multiple restricted messengers from one ControllerMessenger.

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.

Copy link
Contributor

@MajorLift MajorLift Jul 3, 2024

Choose a reason for hiding this comment

The 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 Action or Event type union is completely missing all of the controller's internal actions/events. Interesting...

Co-authored-by: Jongsun Suh <jongsun.suh@icloud.com>
dbrans
dbrans previously approved these changes Jul 3, 2024
packages/gas-fee-controller/CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 10 to 20
## [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))
Copy link
Contributor

Choose a reason for hiding this comment

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

@MajorLift Oh, I just realized you're talking about internal actions/events. Consumers don't need to explicitly allowlist internal actions/events, right? So then this would not be breaking after all. Is that right?

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
dbrans
dbrans previously approved these changes Jul 3, 2024
Co-authored-by: Jongsun Suh <jongsun.suh@icloud.com>
Copy link
Contributor

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks everyone for your patience with the many update requests.

Copy link
Contributor

@mcmire mcmire 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
Copy link
Contributor

ccharly commented Jul 3, 2024

Thanks for all those feedback guys! :)

@ccharly ccharly merged commit a3acb59 into main Jul 3, 2024
116 checks passed
@ccharly ccharly deleted the release/170.0.0 branch July 3, 2024 21:45
sahar-fehri added a commit that referenced this pull request Jul 16, 2024
## Explanation

Releases assets-controllers to new version `36.0.0`

## 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?

For example:

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

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/assets-controllers`

### Added

- **Added**: Add optional `topBid` property to the `NftMetadata` type.
This property must be of type `TopBid`.
([#4522](#4522))
- **Added**: Add optional `floorAsk` property to the `TokenCollection`
type. This property must be of type `FloorAskCollection`.
([#4522](#4522))
- **Added**: Add linea mainnet support to nft detection supported
networks ([#4515](#4515))
- **Added**: Fetch NFT collections data from the NFT-API
`getCollections` API when calling the `detectNfts` method of
`NftDetectionController`, and the `updateNftMetadata` and `watchNft`
methods of `NftController`.
([#4443](#4443))

### Changed

- **Changed**: Bump `@metamask/utils` to `^9.0.0`
([#4508](#4516))
- **Changed**: Bump `@metamask/rpc-errors` to `^6.3.1`
([#4498](#4516))

### Fixed

- **Breaking**: Changed attributes type in `NftMetadata` to
`Attributes[]` ([#4522](#4522))

## 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 highlighted breaking changes using the "BREAKING" category
above as appropriate
@MajorLift MajorLift mentioned this pull request Jul 25, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants