-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Unstoppable Domains IPFS Resolution #16664
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. |
yarn.lock
Outdated
@@ -7293,7 +7309,7 @@ bn.js@>4.0.0, bn.js@^5.1.1, bn.js@^5.1.2, bn.js@^5.1.3, bn.js@^5.2.0, bn.js@^5.2 | |||
resolved "https://registry.yarnpkg.com/bn.js/-/bn.js-5.2.1.tgz#0bc527a6a0d18d0aa8d5b0538ce4a77dccfa7b70" | |||
integrity sha512-eXRvHzWyYPBuB4NBy0cmYQjGitUrtqwbvlzP3G6VFnNRbsZQIxQ10PbKKHt8gZ/HW/D/747aDl+QkDqg3KQLMQ== | |||
|
|||
bn.js@^4.0.0, bn.js@^4.1.0, bn.js@^4.1.1, bn.js@^4.11.0, bn.js@^4.11.6, bn.js@^4.11.7, bn.js@^4.11.8, bn.js@^4.11.9: | |||
bn.js@^4.0.0, bn.js@^4.1.0, bn.js@^4.1.1, bn.js@^4.11.0, bn.js@^4.11.6, bn.js@^4.11.7, bn.js@^4.11.8, bn.js@^4.11.9, bn.js@^4.4.0: |
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.
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.
I'll dig into this Brad and see if we can't get it aligned
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.
Hey Brad!
We tried a couple things to pin the dependency but it looks like the node v16 (dictating the npm version) in the .nvmrc as well as the pinned yarn v1.6 in the package.json engines flag are too outdated to appropriately do this.
Regarding the npm version: It looks like node v16 is using npm v8.17. The NPM overrides call dictated here that can be used to pin nested dependencies is only supported on v8.30 and up.
Regarding the yarn version: We tried doing different variations of this
"resolutions": {
...
"@unstoppabledomains/resolution/**/bn.js": "^4.11.6"
},
But because the unstoppable package name includes a '/' (@unstoppabledomains/resolution) is seems there is a bug in yarn that won't recognize the pin. It was fixed in yarn 2.0 and wasn't backported. related yarn issue here
Very open to any ideas you might have to pin as the devDependency for the resolution library uses bn.js v4.11.6 which is referenced in #16410 so if we can find a way to pin the dependency to that we should be OK.
yarn.lock
Outdated
crypto-js@^4.1.1: | ||
version "4.1.1" | ||
resolved "https://registry.yarnpkg.com/crypto-js/-/crypto-js-4.1.1.tgz#9e485bcf03521041bd85844786b83fb7619736cf" | ||
integrity sha512-o2JlM7ydqd3Qk9CA0L4NL6mTzU2sdx96a+oOfPu8Mkl/PK51vSyoi8/rQ8NknZtk44vq15lmhAj9CIAGwgeWKw== | ||
|
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.
Can you describe what crypto-js is used for in your package? Links to code is fine.
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.
I can ask our dev team for specifics (might not get a clear answer) - I don't know what crypto.js does. Here's a link to their repo. https://github.com/brix/crypto-js
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.
i meant how its being used in these packages, not what the library does, just to clarify
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.
We're using the crypto.js package for hashing algorithms: eip137 (sha3) and zns (sha256) in the resolution library.
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.
const { tabId, url } = details; | ||
console.log('details', details); | ||
// ignore requests that are not associated with tabs | ||
// only attempt ENS resolution on mainnet | ||
// only attempt resolution on mainnet | ||
if (tabId === -1 || getCurrentChainId() !== '0x1') { | ||
return; | ||
} | ||
// parse ens name | ||
// parse domain name | ||
const { hostname: name, pathname, search, hash: fragment } = new URL(url); | ||
// if domain name contains an Unstoppable TLD, it will attempt tp resolve to IPFS | ||
udTlds.forEach((tld) => { | ||
if (name.toLowerCase().endsWith(`.${tld}`)) { | ||
attemptResolveUns(tabId, name); | ||
} | ||
}); | ||
|
||
const domainParts = name.split('.'); | ||
const topLevelDomain = domainParts[domainParts.length - 1]; | ||
// if unsupported TLD, abort | ||
if (!supportedTopLevelDomains.includes(topLevelDomain)) { | ||
if (!supportedEnsTopLevelDomains.includes(topLevelDomain)) { | ||
return; | ||
} | ||
// otherwise attempt resolve | ||
attemptResolve({ tabId, name, pathname, search, fragment }); | ||
// otherwise attempt resolve for ENS | ||
attemptResolveEns({ tabId, name, pathname, search, fragment }); |
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.
@bbehrman10 i'm a noob at IPFS resolution. Will a domain that was registered via ENS be resolved through the 'attemptResolveUns' method? I'm trying to get a sense for if the order you inserted the logic is going to result in a meaningful change to the amount of traffic that MetaMask sends ENS. If its feasible that this change will result in a decrease in ENS usage from within MetaMask then we need to provide a user preference toggle for ENS vs UD resolution and re-order our checks so that it follows that preference. In that case we need to default to ENS for that preference and put in a "whats new" thing explaining that they can now use UD. In any case we can always fall back to the other if it isn't resolved via the user's preference.
So in the case of ENS preferred we attempt ENS resolution and if not then UD, if UD is preferred, reverse the paradigm.
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.
ENS addresses will resolve using the attemptResolveEns (it's the same function as it was before we just added the Ens identifier
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.
it will not impact ENS traffic at all - UD does not resolve the .eth TLD at this time. If that changes, I agree we will need to create an option to toggle preference
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.
my question is if a .eth address is purchased via ENS and then we attempt to resolve it using UD what will the result be?
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.
the way it's written, .eth will always go to ENS and the UD TLDs will all go to UD
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.
Hi Makoto from ENS team. Please be aware that ENS is not just .eth
but the users can import DNS names as ENS, such as .xyz, .io, etc.
https://medium.com/the-ethereum-name-service/announcing-support-for-xyz-on-ens-7f5bc7fe1b24
https://medium.com/the-ethereum-name-service/full-dns-namespace-integration-to-ens-now-on-mainnet-9d37270807d3
Even if UD never extends to .eth
, it has the danger of future conflicts with the DNS imported ENS names whenever ICANN extends tlds (the last extension happened back in 2012 so the new round is expected to happen soon).
"*://*.wallet/", | ||
"*://*.blockchain/", | ||
"*://*.888/", | ||
"*://*.zil/", |
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.
We'll need to look into what changes we might need to make to manifest v3 as well
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.
yeah, weren't sure what was needed in manifest v3
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.
Doesn't look like the ENS ipfs resolution needed to be in manifest v3 but we'll need to defer to you @brad-decker if this is something we need to add.
Hey @brad-decker ! Just wanted to ping incase the above got missed. Looking forward to the feedback! |
Hey @brad-decker ! Just following up. Have you had the opportunity to look through the above? |
Hey @brad-decker. Any update for @bbehrman10 and I? |
New dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No new dependency issues detected in pull request Bot CommandsTo ignore an alert, reply with a comment starting with Pull request alert summary
📊 Modified Dependency Overview:
|
This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions. |
This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions. |
This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions. |
This PR was closed because there has been no follow up activity in the last 14 days. Thank you for your contributions. |
Explanation
Adds IPFS resolution for unstoppable domains. Much like the ENS IPFS resolution with .eth/ but adds support for the Unstoppable TLDs.
Usage
More Information
Spinning off of this pull request: #15817
Pre-merge author checklist
Pre-merge reviewer checklist