-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: replace EnsController with core implementation #23224
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. |
@metamaskbot update-policies |
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
Policies updated |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #23224 +/- ##
===========================================
- Coverage 68.65% 68.61% -0.05%
===========================================
Files 1098 1096 -2
Lines 43185 43091 -94
Branches 11532 11513 -19
===========================================
- Hits 29647 29563 -84
+ Misses 13538 13528 -10 ☔ View full report in Codecov by Sentry. |
Builds ready [28d5e6d]
Page Load Metrics (1149 ± 469 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [f068975]
Page Load Metrics (1327 ± 397 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
@@ -1,29 +0,0 @@ | |||
import { Web3Provider } from '@ethersproject/providers'; | |||
import ensNetworkMap from 'ethereum-ens-network-map'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this package is now no longer imported directly, it can now be removed from dependencies
in package.json
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@legobeat I see that it's still being used here https://github.com/MetaMask/metamask-extension/blob/develop/ui/ducks/domains.js#L3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, interesting. I wonder if there's some duplicate code that we need to get rid or move into EnsController. We can revisit this in a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I'm thinking it could be inlined at this point, making it more obvious what's going on?
- Less duplicate code and more out-of-date configuration by the looks of it: feat(ens-controller): Allow supplying ENS network map in constructor core#4006
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, yes, if we inline the ENS registry map then we can bring it in from ens-controller
for that particular use case. That makes sense.
Builds ready [61c5c74]
Page Load Metrics (1053 ± 302 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Builds ready [9a4a742]
Page Load Metrics (1428 ± 333 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
Following the successful merge of EnsController from the extension to the core Existing EnsController as detailed in MetaMask/core#1129, we should now fully remove the EnsController logic from the extension and instead rely entirely on the core monorepo implementation.
Related issues
Fixes: 23223
Changelog
Pre-merge reviewer checklist