Release 161.0.0

[ccharly]

Explanation

This release contains the following packages:

• @metamask/accounts-controller (major)
• @metamask/assets-controllers (major)
• @metamask/chain-controller (minor)
• @metamask/keyring-controller (minor)
• @metamask/selected-network-controller (patch)
• @metamask/transaction-controller (major)

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

[danroc]

selected-network-controller should be a patch (original PR, cc @jiexi)

[adonesky1]

Entry for @metamask/seelected-network-controller LGTM

[MajorLift]

Hmm we generally don't include devDeps bumps in the changelog, but I'm not sure why keyring-controller isn't a dependency.

That might be a holdover from a past state when keyring-controller only supplied types to accounts-controller?

@mcmire Should we recategorize keyring-controller as a dependency for accounts-controller at some point?

[mcmire]

I'm good with doing that in another PR as well.

This isn't the only case I've seen where a package needed to be moved to a different level. So we (Wallet Framework team) should probably go through all packages and audit them to ensure that any controller package that should be in dependencies and/or peerDependencies should be there. In addition I think knowing how to express dependencies on packages that are only for types — and knowing how to express dependencies that are used in conjunction with the messenger — has been a constant source of confusion, we should add documentation somewhere that explains our position.

[montelaidev]

You're right, this should be moved to dependency now. To not block this release, i'll update this in another PR

[MajorLift]

Sounds good!

[mcmire]

Oh weird my comment got added in the wrong place. Well you got the idea :)

[mcmire]

Saying that the controller allows the actions/events is a bit inaccurate; it would be better to say that the messenger that the controller takes must allow these actions/events. If the consumer isn't doing that currently and upgrades this package, then they will see a type and runtime error when the NftController constructor is run, so that makes this a breaking change:

Suggested change

	- `NftController` now allows `AccountsController:getAccount`, `AccountsController:getSelectedAccount` as messenger actions and subscribes to the `AccountsController:selectedEvmAccountChange` messenger event ([#4221](https://github.com/MetaMask/core/pull/4221))
	- **BREAKING:** The `NftController` messenger must now allow `AccountsController:getAccount` and `AccountsController:getSelectedAccount` as messenger actions and `AccountsController:selectedEvmAccountChange` as a messenger event ([#4221](https://github.com/MetaMask/core/pull/4221))

[ccharly]

Initially we had that BREAKING prefix yes, but after discussing with @MajorLift, I understood this was not considered a "real" breaking changes. Anyway, I do agree with your suggestion, so let's re-add it. :)

[MajorLift]

I agree with Elliot's description of the desired behavior, but it doesn't appear that we have type or runtime error guardrails in controller constructor for subtype messengers. Passing in messengers with incomplete or never allowlists into controller constructors seems to work fine.

Runtime errors are thrown, but only when call and subscribe/unsubscribe (or more accurately #isAllowedAction, #isAllowedEvent) is being invoked for the omitted actions/events.

I'll experiment with this a bit more in the test files since I might just be doing something wrong.

Edit: Created a base-controller ticket for this #4414

[mcmire]

I'm good with doing that in another PR as well.

This isn't the only case I've seen where a package needed to be moved to a different level. So we (Wallet Framework team) should probably go through all packages and audit them to ensure that any controller package that should be in dependencies and/or peerDependencies should be there. In addition I think knowing how to express dependencies on packages that are only for types — and knowing how to express dependencies that are used in conjunction with the messenger — has been a constant source of confusion, we should add documentation somewhere that explains our position.

[mcmire]

Looks good to me!

@MajorLift do you have any more thoughts?

[gantunesr]

Sorry @mcmire and @MajorLift! We missed that last comment (@MajorLift do you have any more thoughts?) before merging

[MajorLift]

@gantunesr No worries, everything looks good to me! :)

[MajorLift]

Ah looks like this belongs under line 14.

Leaving this comment as reference to be fixed by a future PR covering AccountTrackerController.