Update @metamask/controllers to v33
#16493
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gudahtt
force-pushed
the
update-controllers-to-v33
branch
3 times, most recently
from
5967b07 to
0123ab2
Compare
Gudahtt
commented
Nov 14, 2022
worldlyjohn
reviewed
Nov 15, 2022
| @@ -345,14 +345,14 @@ export default class MetamaskController extends EventEmitter { | |||
| this.assetsContractController.getERC1155TokenURI.bind( | |||
| this.assetsContractController, | |||
| ), | |||
| onCollectibleAdded: ({ address, symbol, tokenId, standard, source }) => | |||
| onNftAdded: ({ address, symbol, tokenId, standard, source }) => | |||
| this.metaMetricsController.trackEvent({ | |||
| event: EVENT_NAMES.NFT_ADDED, | |||
There was a problem hiding this comment.
@Gudahtt can you change the event name from EVENT_NAMES.NFT_ADDED to EVENT_NAMES.TOKEN_ADDED? Despite NFT_ADDED being defined in constants/metametrics.js we've never actually used it yet and this would be the preferred way. (ie one event capturing the generalized activity and specific properties to split it out if needed)
There was a problem hiding this comment.
Sounds reasonable! I might do this in a separate PR though, it doesn't seem directly related to the controllers update. This code is pre-existing (possibly still behind a build flag)
The controllers package has been updated to v33. The only breaking change in this release was to rename the term "collectible" to "NFT" wherever it appeared in the API. Changes in this PR have been kept minimal; additional renaming can be done in separate PRs. This PR only updates the controller names, controller state, controller methods, and any direct references to these things. NFTs are still called "collectibles" in most places.
Gudahtt
force-pushed
the
update-controllers-to-v33
branch
from
e2f4ae6 to
c1bbb82
Compare
darkwing
approved these changes
Nov 15, 2022
adonesky1
approved these changes
Nov 15, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The controllers package has been updated to v33. The only breaking change in this release was to rename the term "collectible" to "NFT" wherever it appeared in the API.
Changes in this PR have been kept minimal; additional renaming can be done in separate PRs. This PR only updates the controller names, controller state, controller methods, and any direct references to these things. NFTs are still called "collectibles" in most places.
Manual Testing Steps
To test the migration, you can setup a wallet using a build from
develop, then switch to a build on this branch. You can compare the wallet state before and after to ensure the migration was correctly applied.Aside from the migration, there are functional changes, so any standard regression testing steps would apply. Maybe pay special attention to any NFT-related functionality though.
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.