Merge extension ens controller by cryptodev-2s · Pull Request #1170 · MetaMask/core

cryptodev-2s · 2023-04-06T16:26:34Z

This PR merges the extension ens controller with the core ens controller
Fix issue #1129

mcmire

It seems like even if we're bringing code from the extension we can use this opportunity to clean things up a bit. My suggestions below are mainly geared toward that end. However it also seems that there are other changes here that we might not want just yet.

packages/controller-utils/src/constants.ts

packages/network-controller/src/NetworkController.ts

packages/controller-utils/src/constants.ts

packages/ens-controller/src/EnsController.ts

legobeat · 2023-04-11T04:00:29Z

Related: MetaMask/metamask-extension#18035
As noted there, ENS lookups in the extension are currently limited to predefined networks, which is addressed in that PR.

Something to consider bringing in here as well, or better saved for a follow-up?

socket-security · 2023-04-11T16:04:59Z

New dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary

Issue	Status
Install scripts	✅ 0 issues
Native code	✅ 0 issues
Bin script shell injection	✅ 0 issues
Unresolved require	✅ 0 issues
Invalid package.json	✅ 0 issues
HTTP dependency	✅ 0 issues
Git dependency	✅ 0 issues
Potential typo squat	✅ 0 issues
Known Malware	✅ 0 issues
Telemetry	✅ 0 issues
Protestware/Troll package	✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package	Capability Access	`+/-` Transitive Count	Publisher
ethereum-ens-network-map@1.0.2	None	`+0`	danfinlay

mcmire · 2023-04-11T16:39:02Z

@legobeat Thanks for linking that PR, I wasn't aware of that. I looked through the PR and it doesn't seem to change the controller itself, so I think we should take care of that in a followup PR.

mcmire · 2023-04-13T16:38:42Z

@cryptodev-2s Just FYI, looks like the newest changes are failing the lint step. There's also the outstanding questions I have above. Let me know if my comments make sense or if you want to meet briefly to discuss them.

packages/ens-controller/src/EnsController.ts

mcmire · 2023-04-17T21:56:08Z

@cryptodev-2s If you push up the changes you were showing me today then I can help you tweak this further!

packages/ens-controller/src/EnsController.ts

packages/controller-utils/src/constants.ts

packages/ens-controller/src/EnsController.ts

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

packages/transaction-controller/src/TransactionController.test.ts

packages/ens-controller/package.json

Gudahtt

LGTM!

Edit: Oh, lint failures. I can re-approve after those are fixed.

packages/ens-controller/src/EnsController.test.ts

localhost removed

The network change handler for the ENS controller has been broken since the PR #1170 due to a conflict with #1196, which was merged around the same time. It referenced the `network` property of the network state that we have removed. The change handler has been updated to use `networkId` instead. Additionally, the `NetworkState` type has been imported so that we're less likely to make this mistake again.

Merge extension ens controller

The network change handler for the ENS controller has been broken since the PR #1170 due to a conflict with #1196, which was merged around the same time. It referenced the `network` property of the network state that we have removed. The change handler has been updated to use `networkId` instead. Additionally, the `NetworkState` type has been imported so that we're less likely to make this mistake again.

Merge extension ens controller

The network change handler for the ENS controller has been broken since the PR #1170 due to a conflict with #1196, which was merged around the same time. It referenced the `network` property of the network state that we have removed. The change handler has been updated to use `networkId` instead. Additionally, the `NetworkState` type has been imported so that we're less likely to make this mistake again.

cryptodev-2s added the DO-NOT-MERGE label Apr 6, 2023

cryptodev-2s requested a review from a team as a code owner April 6, 2023 16:26

mcmire linked an issue Apr 6, 2023 that may be closed by this pull request

Merge the extension ENS controller with the core ENS controller #1129

Closed

4 tasks

mcmire reviewed Apr 6, 2023

View reviewed changes

legobeat mentioned this pull request Apr 11, 2023

Ens multicoin MetaMask/metamask-extension#18035

Closed

6 tasks

cryptodev-2s force-pushed the feature/merge-extension-ens-controller branch from 6f8caf4 to 8b80cd4 Compare April 11, 2023 16:03

cryptodev-2s removed the DO-NOT-MERGE label Apr 11, 2023

cryptodev-2s requested review from legobeat and mcmire April 12, 2023 15:57

cryptodev-2s force-pushed the feature/merge-extension-ens-controller branch from b408e1f to 5f5847f Compare April 12, 2023 16:10

Gudahtt reviewed Apr 17, 2023

View reviewed changes

packages/ens-controller/src/EnsController.ts Outdated Show resolved Hide resolved

packages/ens-controller/src/EnsController.ts Outdated Show resolved Hide resolved

packages/ens-controller/src/EnsController.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 17, 2023

View reviewed changes

packages/ens-controller/src/EnsController.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 17, 2023

View reviewed changes

packages/ens-controller/src/EnsController.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 17, 2023

View reviewed changes

packages/ens-controller/src/EnsController.ts Show resolved Hide resolved

packages/ens-controller/src/EnsController.ts Outdated Show resolved Hide resolved

cryptodev-2s force-pushed the feature/merge-extension-ens-controller branch 4 times, most recently from 42346e8 to 704b617 Compare April 18, 2023 14:58

cryptodev-2s commented Apr 18, 2023

View reviewed changes

packages/ens-controller/src/EnsController.ts Outdated Show resolved Hide resolved

cryptodev-2s force-pushed the feature/merge-extension-ens-controller branch from 69af306 to ec95052 Compare April 18, 2023 18:04

cryptodev-2s requested a review from Gudahtt April 18, 2023 18:04

Gudahtt reviewed Apr 18, 2023

View reviewed changes

cryptodev-2s changed the title ~~feat: init merge ens controller~~ feat: merge extension ens controller Apr 18, 2023

cryptodev-2s changed the title ~~feat: merge extension ens controller~~ Merge extension ens controller Apr 18, 2023

cryptodev-2s requested a review from Gudahtt April 18, 2023 22:29

cryptodev-2s and others added 4 commits April 20, 2023 02:11

Remove unnecessary cast using a type guard (#1208)

e5c840a

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

Fix yarn lock

b1c5c82

Use hasProperty from controllers utils

af8d4ff

Clear ensResolutionsByAddress on networkStateChange

da085eb

Gudahtt reviewed Apr 20, 2023

View reviewed changes

packages/transaction-controller/src/TransactionController.test.ts Outdated Show resolved Hide resolved

cryptodev-2s force-pushed the feature/merge-extension-ens-controller branch from 08f49ec to f76e0d5 Compare April 20, 2023 02:28

cryptodev-2s requested a review from Gudahtt April 20, 2023 02:28

Make ethProvider private

c04737f

cryptodev-2s force-pushed the feature/merge-extension-ens-controller branch from f76e0d5 to c04737f Compare April 20, 2023 02:29

Gudahtt reviewed Apr 20, 2023

View reviewed changes

packages/ens-controller/package.json Outdated Show resolved Hide resolved

Gudahtt previously approved these changes Apr 20, 2023

View reviewed changes

Use latest @metamask/utils

2aac5b3

cryptodev-2s dismissed Gudahtt’s stale review via 2aac5b3 April 20, 2023 02:38

Gudahtt approved these changes Apr 20, 2023

View reviewed changes

cryptodev-2s commented Apr 20, 2023

View reviewed changes

packages/ens-controller/src/EnsController.test.ts Show resolved Hide resolved

cryptodev-2s added 2 commits April 20, 2023 03:35

Merge branch 'main' into feature/merge-extension-ens-controller

45a4045

Merge branch 'main' into feature/merge-extension-ens-controller

ff2643c

cryptodev-2s merged commit 7d5057b into main Apr 20, 2023

cryptodev-2s deleted the feature/merge-extension-ens-controller branch April 20, 2023 16:45

legobeat mentioned this pull request Apr 25, 2023

Release 49.0.0 #1263

Merged

Gudahtt mentioned this pull request May 10, 2023

Fix ENS controller network change handler #1362

Merged

3 tasks

MajorLift pushed a commit that referenced this pull request Oct 11, 2023

Merge extension ens controller (#1170)

602be21

Merge extension ens controller

MajorLift pushed a commit that referenced this pull request Oct 11, 2023

Merge extension ens controller (#1170)

eff6c2a

Merge extension ens controller

Uh oh!

Conversation

cryptodev-2s commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcmire left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

legobeat commented Apr 11, 2023

Uh oh!

socket-security bot commented Apr 11, 2023

Uh oh!

mcmire commented Apr 11, 2023

Uh oh!

mcmire commented Apr 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcmire commented Apr 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Gudahtt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cryptodev-2s commented Apr 6, 2023 •

edited

Loading

mcmire left a comment •

edited

Loading

Gudahtt left a comment •

edited

Loading