Skip to content
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: always use snaps to resolve domains; include preinstalled ENS resolver snap #26242

Merged
merged 42 commits into from
Sep 18, 2024

Conversation

mirceanis
Copy link
Contributor

@mirceanis mirceanis commented Jul 31, 2024

Description

The embedded ENS resolution logic is being replaced by snaps that use endowment:name-lookup.
This change-set also includes a preinstalled snap that does ENS resolution including ENS multi-coin functionality.

The ENS resolver snap will use the Ethereum provider from the extension on Ethereum mainnet and ENS supported testnets and will revert to an Infura JSON-RPC when the extension is connected to other networks. On other networks it tries to resolve the network-specific ENS address and if none exists will try to resolve the mainnet address (with a ⚠️ warning) as long as that address appears to be an externally owned account.

Most of the relevant changes are in domains.js.
Since the preinstalled snap uses ethers v6 while the extension depends on ethers v5.7 there are some changes in the lavamoat policies. Also, the more recent ethers version requires a different set of mocks, requiring a small change in the e2e test.

Open in GitHub Codespaces

Related issues

fixes https://github.com/MetaMask/MetaMask-planning/issues/2403
closes #18035
fixes #18648
fixes #22797
fixes #8556
fixes MetaMask/specifications#11

Manual testing steps

  1. While on Ethereum mainnet, start a send flow
  2. Type an ENS name in the recipient field(example vitalik.eth), pick the resulting address. (this should be identical to previous behavior)
  3. Switch to a L2
  4. Type an EOA ENS name in the recipient field (example vitalik.eth), observe an address being resolved with a ⚠️warning.
  5. Type an ENS name that would resolve to a contract on mainnet (example uniswap.eth). Observe No resolution for domain provided. error.

Screenshots/Recordings

Before

After

after.mov

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

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.

@mirceanis mirceanis force-pushed the feat/2403-resolve-on-l2 branch 2 times, most recently from 5962a95 to 81fb9c0 Compare July 31, 2024 14:01
@mirceanis mirceanis force-pushed the feat/2403-resolve-on-l2 branch from 81fb9c0 to 19fbf60 Compare July 31, 2024 16:21
Copy link

socket-security bot commented Jul 31, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: npm/@metamask/[email protected]

View full report↗︎

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

@mirceanis mirceanis force-pushed the feat/2403-resolve-on-l2 branch from 70e86ff to 5cf7905 Compare August 1, 2024 08:59
@mirceanis
Copy link
Contributor Author

@SocketSecurity ignore npm/@metamask/[email protected]

@mirceanis mirceanis force-pushed the feat/2403-resolve-on-l2 branch from 534452a to 962b056 Compare August 1, 2024 10:27
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 20 lines in your changes missing coverage. Please review.

Project coverage is 70.05%. Comparing base (ca6fd53) to head (0afeb71).
Report is 23 commits behind head on develop.

Files with missing lines Patch % Lines
ui/ducks/domains.js 5.26% 18 Missing ⚠️
...s/send/components/domain-input-resolution-cell.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #26242      +/-   ##
===========================================
+ Coverage    69.96%   70.05%   +0.09%     
===========================================
  Files         1442     1442              
  Lines        50100    50009      -91     
  Branches     14006    13965      -41     
===========================================
- Hits         35049    35029      -20     
+ Misses       15051    14980      -71     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -560,6 +561,7 @@
"eslint-plugin-react-hooks": "^4.2.0",
"eslint-plugin-storybook": "^0.6.15",
"eta": "^3.2.0",
"ethers": "5.7.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not directly used, but expected by @account-abstraction/contracts during tests.
Without this, tests would fail as the transitive ethers v6+ would get used.

ui/ducks/domains.js Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [83e07e3]
Page Load Metrics (539 ± 370 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint7749016911354
domContentLoaded9148403416
load482103539770370
domInteractive9148403416
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -105 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: -849 Bytes (-0.01%)

@mirceanis mirceanis marked this pull request as ready for review August 2, 2024 14:17
@mirceanis mirceanis requested review from a team as code owners August 2, 2024 14:17
…2403-resolve-on-l2

# Conflicts:
#	lavamoat/browserify/beta/policy.json
#	lavamoat/browserify/flask/policy.json
#	lavamoat/browserify/main/policy.json
#	lavamoat/browserify/mmi/policy.json
@metamaskbot
Copy link
Collaborator

Builds ready [546b2bc]
Page Load Metrics (319 ± 271 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint672741266632
domContentLoaded9164415024
load401735319563271
domInteractive9164415024
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 10.63 KiB (0.30%)
  • ui: 0 Bytes (0.00%)
  • common: 24.74 KiB (0.35%)

jonybur
jonybur previously approved these changes Aug 14, 2024
@mirceanis mirceanis force-pushed the feat/2403-resolve-on-l2 branch from e88bd56 to 572c18a Compare September 13, 2024 10:53
@danjm
Copy link
Contributor

danjm commented Sep 17, 2024

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

No policy changes

@metamaskbot
Copy link
Collaborator

Builds ready [0afeb71]
Page Load Metrics (1746 ± 80 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint28820941660349168
domContentLoaded14822068173316881
load15002084174616680
domInteractive246533126
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 249.42 KiB (7.07%)
  • ui: 74.95 KiB (1.04%)
  • common: 91.5 KiB (1.14%)

…2403-resolve-on-l2

# Conflicts:
#	lavamoat/browserify/beta/policy.json
#	lavamoat/browserify/flask/policy.json
#	lavamoat/browserify/main/policy.json
#	lavamoat/browserify/mmi/policy.json
Signed-off-by: Mircea Nistor <[email protected]>
Copy link

sonarcloud bot commented Sep 18, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [2dcdfe4]
Page Load Metrics (1744 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint23519301591454218
domContentLoaded15091921171910349
load15192008174411153
domInteractive13151402814
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 249.42 KiB (7.05%)
  • ui: 74.95 KiB (1.04%)
  • common: 91.5 KiB (1.14%)

Copy link
Contributor

@hmalik88 hmalik88 left a comment

Choose a reason for hiding this comment

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

LGTM

@mirceanis mirceanis mentioned this pull request Sep 18, 2024
7 tasks
Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

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

Change in files owned by confirmations team look good.

Copy link
Member

@weizman weizman left a comment

Choose a reason for hiding this comment

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

lgtm (strictly policies):

  • new globals:
    • fetch and WebSocket introduced by a different version of the pkg (so they're not new)
    • define is also already used previously
    • could not find other new globals that may be risky
  • new packages
    • packages consumption seems fine too, just a reorg
  • I only checked main/policy.json, assuming the rest are similar (worth a check @mirceanis)

@mirceanis mirceanis merged commit 5e08c06 into develop Sep 18, 2024
89 checks passed
@mirceanis mirceanis deleted the feat/2403-resolve-on-l2 branch September 18, 2024 16:09
@github-actions github-actions bot locked and limited conversation to collaborators Sep 18, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 18, 2024
@metamaskbot metamaskbot added release-12.5.0 Issue or pull request that will be included in release 12.5.0 and removed release-12.6.0 Issue or pull request that will be included in release 12.6.0 labels Sep 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-multichain area-snaps release-12.5.0 Issue or pull request that will be included in release 12.5.0 team-identity team-wallet-ux
Projects
Archived in project
9 participants