Migrate to new controller packages #16547
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
`@metamask/controllers` is deprecated, and most of the controllers that lived here are now located in their own package ([1]). This commit replaces `@metamask/controllers` in `package.json` with references to these packages and updates `import` lines to match. [1]: MetaMask/core#831
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
mcmire
commented
Nov 17, 2022
|
|
||
| # Test @metamask package publishing | ||
| .npmrc |
There was a problem hiding this comment.
This is a temporary change and I can bring this out into a separate PR. For now, I've explained how this is used in the PR description. We want to gitignore it because if it's used for testing we don't want people's access tokens to end up in the repo.
There was a problem hiding this comment.
Well, I guess if we have the CircleCI config in this branch, we can have this here too since they're related.
package.json
Outdated
| @@ -110,24 +110,35 @@ | |||
| "@keystonehq/bc-ur-registry-eth": "^0.12.1", | |||
| "@keystonehq/metamask-airgapped-keyring": "^0.6.1", | |||
| "@material-ui/core": "^4.11.0", | |||
| "@metamask/address-book-controller": "1.0.0-f2fa748", | |||
There was a problem hiding this comment.
The versions we are using here look a bit funky, but they're autogenerated based on this PR and are temporary until we are able to QA the new packages in both extension and mobile. Once we've done that, and before merging this PR, I'll replace them with "1.0.0".
| @@ -27,6 +27,9 @@ export const GAS_LIMITS = { | |||
| * These are already declared in @metamask/controllers but importing them from | |||
There was a problem hiding this comment.
I'm not sure if this comment will be true after this change. If there's no reason why it wouldn't be I can remove the TODO here and we can revisit this later.
Gudahtt
force-pushed
the
migrate-to-new-controller-packages
branch
from
8fe18a4
to
cd9ea6e
Compare
|
Marking as |
3 tasks
|
Moving this back to draft so that we can continue to access the preview versions of the aforementioned packages within the GitHub Package Registry, but please feel free to review this! |
Builds ready [ea58338]
Page Load Metrics (2001 ± 101 ms)
Bundle size diffs [🚀 Bundle size reduced!]
highlights:storybook
|
Builds ready [2f7761c]
Page Load Metrics (2060 ± 126 ms)
Bundle size diffs [🚀 Bundle size reduced!]
highlights:storybook
|
|
The controller packages have been published! This is now ready for review. |
Builds ready [ca210aa]
Page Load Metrics (2202 ± 147 ms)
Bundle size diffs [🚀 Bundle size reduced!]
highlights:storybook
|
brad-decker
previously approved these changes
Nov 22, 2022
|
nevermind! Build fluke. Got it working. |
adonesky1
previously approved these changes
Nov 22, 2022
|
The PR looks good. I confirm it fixes the issues with 0 decimal tokens. I've QAd on Chrome, as on FireFox the extension is crashing (due to the FireFox bug spot a week ago, now fixed on develop). I only found this issue, but I cannot ensure it is due to this PR, nor how to reproduce it consistently. |
…troller-packages * origin/develop: (21 commits) Simplify MV3 initialization (#16559) Fix #15050 - MV3: Keep the user logged in when service worker restarts (#15558) Integrating snow into metamask (#15580) fix infura rpc detection (#16585) Remove callback from being saved in controller state (#16627) Icon house keeping updates (#16621) Fix bundle size diff message (#16576) avatar base component housekeeping (#16583) Disabled save button on add contact form if input fields are empty (#16233) Fix E2E chunking (#16653) fixed console warning for labelProps (#16629) Adding `FormTextField` component (#16497) Fix/16620/button href prop (#16633) Update to correct version Beta Changelog Fix version number Ensure prod beta build is created when merging to master (#16557) Beta: Update long logo to new imagery (#16505) BETA - Add beta banner to all screens (#16307) BETA - Update logo imagery (#16304) ...
Builds ready [ec2be79]
Page Load Metrics (2159 ± 113 ms)
Bundle size diffs [🚀 Bundle size reduced!]
highlights:storybook
|
|
Thanks @seaona ! I took a look at that issue, and it looks like it's functioning as expected, but with incorrect logging. I have brought this branch up-to-date with |
|
thank you @Gudahtt !! I've pulled the last changes and looking good on FF too |
brad-decker
approved these changes
Nov 24, 2022
adonesky1
approved these changes
Nov 24, 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.
Explanation
@metamask/controllersis deprecated, and most of the controllers that lived here are now located in their own package. This commit replaces@metamask/controllersinpackage.jsonwith references to these packages and updatesimportlines to match.Manual Testing Steps
This is a behind-the-scenes change, and there should be no changes to existing functionality — everything should work as it does currently. I'm not sure what the best protocol to test these sorts of changes is (maybe run through the most common scenarios? test-dapp?)
Note that before this PR gets merged, preview versions of the new packages, which live on GitHub Packages, will be used. Without specially configuring NPM, these packages will not be accessible locally, and therefore you will not be able to run
yarn setup. To get this working:.npmrcin the root directory with the following content (note: this file is gitignored, so don't worry about it ending up in the repo):Now you should be able to run
yarn setup, thenyarn start, and load the extension locally.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.