Bump @metamask/assets-controller to v20.0.0#22152
Conversation
|
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. |
|
Updated dependencies detected. Learn more about Socket for GitHub ↗︎
|
yarn.lock
Outdated
There was a problem hiding this comment.
This dependency becomes duplicated here and @metamask/keyring-controller is still staying on @metamask/preferences-controller@4.4.3. Is this as intended?
There was a problem hiding this comment.
Not sure I follow. This bump occurs in @metamask/assets-controllers v20.0.0: https://github.com/MetaMask/core/blob/ae1c9ebea94904ef0a5dd5c7c701a69a9f2c3069/packages/assets-controllers/CHANGELOG.md?plain=1#L35
There was a problem hiding this comment.
Before this PR, this repo has:
@metamask/preferences-controller@4.4.3- via
@metamask/assets-controller@19.0.0 - via
@metamask/keyring-controller@8.1.0
- via
After:
@metamask/preferences-controller@4.4.3- via
@metamask/keyring-controller@8.1.0
- via
@metamask/preferences-controller@5.0.0- via
@metamask/assets-controller@20.0.0
- via
Highlighting to ensure that this has been considered and the update here doesn't mean that the peerDependencies of either of these get violated due to expecting one version and getting passed an incompatible one.
There was a problem hiding this comment.
🤔 But they're different major versions? Won't @metamask/keyring-controller@8.1.0 just continue to get @metamask/preferences-controller@4.4.3 and now @metamask/preferences-controller@4.4.3 will use @metamask/preferences-controller@5.0.0?
We have 3 different major versions of @metamask/eth-sig-util in the extension's dependency graph for example. Each dep gets the right version as a sub dep:
There was a problem hiding this comment.
@adonesky1 the concern that @legobeat is voicing isn't that they operate on different code, but rather that values or instances in the extension get passed around that might differ in shape for the two various preferences controllers. As an example imagine if each of these controllers (assets and keyring) required an instance of Preferences Controller to be supplied to it. If the differences in the preferences controller was breaking its reasonable to assume that the instance passed in to these two controllers would not satisfy both.
I don't think this is the case here, but this is what @legobeat is trying to be assured of. It doesn't necessarily have to be passed in either. If each instance of preferences controller required the presence of a named post message stream to exist on the extension, and the major version changed the way this stream works with data, then it would fundamentally cause a problem because each version of the preferences controller would be looking for different signatures.
again not saying that is happening here :) he just wants us to be sure that these differing versions have been accounted for. In addition we should do our best to not duplicate dependencies as this causes bloat in bundle size as you are well aware given all of your past work trying to mitigate this issue. So its good to note and call out.
There was a problem hiding this comment.
Gotcha. Yeah I see now. Well hilariously in this case the extension uses its own version of the PreferenceController. And definitely no instance of the controller is passed around. The breaking change that caused the bump to version 5.0.0 was adding the showIncomingTransactions value to state which is never referenced anywhere in @metamask/assets-controllers so I think we're all good.
There was a problem hiding this comment.
Thanks for sharing my perspective @brad-decker 😸
Secondary concern to the main point above but it also looks like think the ~34kb bundle-size increase incurred here could be undone if they get back on the same version.
92a369f to
9e51ccd
Compare
9e51ccd to
7b1d06e
Compare
7b1d06e to
44c0fdb
Compare
Builds ready [2b21415]
Page Load Metrics (1333 ± 83 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #22152 +/- ##
===========================================
- Coverage 67.71% 67.67% -0.05%
===========================================
Files 1052 1052
Lines 40859 40876 +17
Branches 10945 10950 +5
===========================================
- Hits 27667 27660 -7
- Misses 13192 13216 +24 ☔ View full report in Codecov by Sentry. |
|
Missing release label release-11.7.3 on PR. Adding release label release-11.7.3 on PR and removing other release labels(release-11.8.0), as PR was cherry-picked in branch 11.7.3. |
Bumps
@metamask/assets-controllerto v20.0.0.Adds
getNetworkClientByIdcallback toTokenRatesControllerconstructor to address only breaking changeThis overwrites and deletes a patch that was added on top of
@metamask/assets-controllersversion 19.0.0. This was added for the 11.7.0 extension release but can be removed now and will be added back later via MetaMask/core#3619 (cc @DDDDDanica @Gudahtt)