Skip to content

Merge extension ens controller - #1170

Merged
cryptodev-2s merged 13 commits into
mainfrom
feature/merge-extension-ens-controller
Apr 20, 2023
Merged

Merge extension ens controller#1170
cryptodev-2s merged 13 commits into
mainfrom
feature/merge-extension-ens-controller

Conversation

@cryptodev-2s

@cryptodev-2scryptodev-2s commented Apr 6, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@cryptodev-2s
cryptodev-2s requested a review from a team as a code ownerApril 6, 2023 16:26
@mcmiremcmire 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
mcmire reviewed Apr 6, 2023
View reviewed changes

@mcmiremcmire left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment threadpackages/controller-utils/src/constants.ts Outdated
Comment threadpackages/network-controller/src/NetworkController.ts Outdated
Comment threadpackages/controller-utils/src/constants.ts Outdated
Comment threadpackages/ens-controller/src/EnsController.ts Outdated
Comment threadpackages/ens-controller/src/EnsController.ts Outdated
Comment threadpackages/ens-controller/src/EnsController.ts Outdated
Comment threadpackages/ens-controller/src/EnsController.ts Outdated
Comment threadpackages/ens-controller/src/EnsController.ts Outdated
Comment threadpackages/ens-controller/src/EnsController.ts Outdated
Comment threadpackages/ens-controller/src/EnsController.ts Outdated
@legobeat

legobeat commented Apr 11, 2023

Copy link
Copy Markdown
Contributor

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?

@legobeatlegobeat mentioned this pull request Apr 11, 2023
Closed
6 tasks
@cryptodev-2s
cryptodev-2sforce-pushed the feature/merge-extension-ens-controller branch from 6f8caf4 to 8b80cd4CompareApril 11, 2023 16:03
@socket-security

socket-securityBot commented Apr 11, 2023

Copy link
Copy Markdown

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
    IssueStatus
    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 PackageCapability Access+/- Transitive CountPublisher
    ethereum-ens-network-map@1.0.2None+0danfinlay

    @mcmire

    mcmire commented Apr 11, 2023

    Copy link
    Copy Markdown
    Collaborator

    @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.

    @cryptodev-2s
    cryptodev-2sforce-pushed the feature/merge-extension-ens-controller branch from b408e1f to 5f5847fCompareApril 12, 2023 16:10
    @mcmire

    mcmire commented Apr 13, 2023

    Copy link
    Copy Markdown
    Collaborator

    @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.

    Gudahtt
    Gudahtt reviewed Apr 17, 2023
    View reviewed changes
    Comment threadpackages/ens-controller/src/EnsController.ts Outdated
    Comment threadpackages/ens-controller/src/EnsController.ts Outdated
    Comment threadpackages/ens-controller/src/EnsController.ts Outdated
    Gudahtt
    Gudahtt reviewed Apr 17, 2023
    View reviewed changes
    Comment threadpackages/ens-controller/src/EnsController.ts Outdated
    Gudahtt
    Gudahtt reviewed Apr 17, 2023
    View reviewed changes
    Comment threadpackages/ens-controller/src/EnsController.ts Outdated
    Gudahtt
    Gudahtt reviewed Apr 17, 2023
    View reviewed changes
    Comment threadpackages/ens-controller/src/EnsController.ts Outdated
    Comment threadpackages/ens-controller/src/EnsController.ts Outdated
    @mcmire

    mcmire commented Apr 17, 2023

    Copy link
    Copy Markdown
    Collaborator

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

    @cryptodev-2s
    cryptodev-2sforce-pushed the feature/merge-extension-ens-controller branch 4 times, most recently from 42346e8 to 704b617CompareApril 18, 2023 14:58
    cryptodev-2s
    cryptodev-2s commented Apr 18, 2023
    View reviewed changes
    Comment threadpackages/ens-controller/src/EnsController.ts Outdated
    @cryptodev-2s
    cryptodev-2sforce-pushed the feature/merge-extension-ens-controller branch from 69af306 to ec95052CompareApril 18, 2023 18:04
    Gudahtt
    Gudahtt reviewed Apr 18, 2023
    View reviewed changes
    Comment threadpackages/controller-utils/src/constants.ts Outdated
    Comment threadpackages/ens-controller/src/EnsController.ts
    Comment threadpackages/ens-controller/src/EnsController.ts Outdated
    Comment threadpackages/ens-controller/src/EnsController.ts Outdated
    @cryptodev-2scryptodev-2s changed the title feat: init merge ens controllerfeat: merge extension ens controllerApr 18, 2023
    @cryptodev-2scryptodev-2s changed the title feat: merge extension ens controllerMerge extension ens controllerApr 18, 2023
    Gudahtt
    Gudahtt reviewed Apr 20, 2023
    View reviewed changes
    Comment threadpackages/transaction-controller/src/TransactionController.test.ts Outdated
    @cryptodev-2s
    cryptodev-2sforce-pushed the feature/merge-extension-ens-controller branch from 08f49ec to f76e0d5CompareApril 20, 2023 02:28
    @cryptodev-2s
    cryptodev-2sforce-pushed the feature/merge-extension-ens-controller branch from f76e0d5 to c04737fCompareApril 20, 2023 02:29
    Gudahtt
    Gudahtt reviewed Apr 20, 2023
    View reviewed changes
    Comment threadpackages/ens-controller/package.json Outdated
    Gudahtt
    Gudahtt previously approved these changes Apr 20, 2023
    View reviewed changes

    @GudahttGudahtt left a comment •
    edited
    Loading

    Copy link
    Copy Markdown
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM!

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

    cryptodev-2s
    cryptodev-2s commented Apr 20, 2023
    View reviewed changes
    Comment threadpackages/ens-controller/src/EnsController.test.ts
    @cryptodev-2s
    cryptodev-2s merged commit 7d5057b into mainApr 20, 2023
    @cryptodev-2s
    cryptodev-2s deleted the feature/merge-extension-ens-controller branch April 20, 2023 16:45
    @legobeatlegobeat mentioned this pull request Apr 25, 2023
    Merged
    Gudahtt added a commit that referenced this pull request May 10, 2023
    @Gudahtt
    Fix ENS controller network change handler
    0bb0b54
    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.
    @GudahttGudahtt mentioned this pull request May 10, 2023
    Merged
    3 tasks
    Gudahtt added a commit that referenced this pull request May 10, 2023
    @Gudahtt
    Fix ENS controller network change handler (#1362)
    ea31aa5
    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.
    Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @GudahttGudahttGudahtt approved these changes
    @mcmiremcmireAwaiting requested review from mcmire
    @legobeatlegobeatAwaiting requested review from legobeat

    Assignees

    No one assigned

    Labels

    None yet

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    Merge the extension ENS controller with the core ENS controller

    4 participants

    @cryptodev-2s@legobeat@mcmire@Gudahtt