Disaggregate the detection param for addNft()

[adonesky1]

This PR decouples the detection parameter passed to the addNft method on the NftController, introducing more granular control over the logic within the function. Now instead of receiving a single detection argument as { chainId, accountAddress } the presence of which was meant to both indicate that the nft being added was detected and which account/chainId to add the NFT to in state, there are two separate arguments accountParams and `source.

This allows us to be more clear and explicit about the source of the NFT being added (manually, via dapp, or auto-detection) which in turn allows us finer tuned logic control and event handling for those different sources.

Changelog entries:

@metamask/approval-controller

• FIXED: Fix ApprovalController constructor so that it accepts a messenger created via a getRestricted without explicitly specifying type parameters

@metamask/assets-controller

• ADDED: Add a fifth parameter to addNft in NftController which can be used to specify whether the NFT was detected, added manually, or suggested by a dapp
• FIXED: Fix watchNft in NftController to ensure that if the network changes before the user accepts the request, the NFT is added to the chain ID and address before the request was initiated

[mcmire]

Makes sense. Just one question.

[mcmire]

Are there existing tests covering this? If so do they need to be extended to verify that these changes produce the desired effect?

[adonesky1]

so I have a PR pointed at this one to test atleast part of the additions here.

You're right we should test this a bit more explicitly though.

[mcmire]

Ah gotcha, okay. I'll take a look at that one.

[adonesky1]

more tests added here: 7b54bcf

[mcmire]

Ah. That's good - but are there any tests that need to be added for NftDetectionController specifically?

[adonesky1]

I've added one here: 4e9b1ee#diff-c48a7b91e7a594a25f0868505f6dd588bbfc783890f0ee2c381399daeba3e04eR316

NftDetection behavior should be unchanged but we should have had this test before.

[mcmire]

Ah I see. Cool. Thank you!

[mcmire]

Looks good!

[mcmire]

I had just one suggestion, but this otherwise looks good.

Would you mind adding a little changelog section to the PR description? That helps us when compiling the changelog for the new release. I'd imagine it would be something like:

## `@metamask/approval-controller`

- **FIXED:** Fix ApprovalController constructor so that it accepts a messenger created via a `getRestricted` without explicitly specifying type parameters

## `@metamask/assets-controller`

- **ADDED:** Add a fifth parameter to `addNft` in NftController which can be used to specify whether the NFT was detected, added manually, or suggested by a dapp
- **FIXED:** Fix `watchNft` in NftController to ensure that if the network changes before the user accepts the request, the NFT is added to the chain ID and address before the request was initiated
- **FIXED:** Fix `detectNfts` in NftDetectionController 

[mcmire]

Since this type is only used within assets-controllers, can it live inside there? Also, I wonder if this name is too generic — I can see it clashing with something else in the future. What do you think about NftSource?

[adonesky1]

Sure! I was thinking that it might be useful for other controllers (particularly for metrics). But yeah I suppose those other controllers that would use it would probably be in the assets-controllers package. Do you think we should create a shared constants file for that package? And regarding the name I was thinking we keep it generic again because it can probably be used for other non-nft specific cases in the future. But perhaps we can just rename it when it gets used as such?

[adonesky1]

did something like this here: 56466f2

[mcmire]

Sorry I was caught up in meetings today. I think that's better!