-
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
Integration ENS with IPFS #4405
Conversation
@PhyrexTsai this is awesome, please give us time to review it! |
@kumavis Thank you! Please let me know if there's anything needed to be changed. |
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.
Just commenting for now, there are a few other opinions I'd like to get on parts of this.
<body> | ||
<div class="app"> | ||
<img src="./images/404.png" alt=""> | ||
<h2>Powered by <a href="https://www.portal.network/">Portal Network</a></h2> |
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 wasn't sure how I felt about this at first, but you know, getting credit for what you do seems pretty fair. I'm fine leaving it up, at least for a while. The same way we link to 409H for the phishing detector.
app/manifest.json
Outdated
}, | ||
"content_security_policy": "script-src 'self' 'unsafe-eval'; object-src 'self'" |
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.
Is there a way to not include the unsafe-eval
permission?
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.
Just remove it.
app/manifest.json
Outdated
@@ -1,7 +1,7 @@ | |||
{ | |||
"name": "__MSG_appName__", | |||
"short_name": "__MSG_appName__", | |||
"version": "4.6.0", | |||
"version": "4.6.2", |
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.
Please leave the version bumping to us.
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.
Sure.
app/manifest.json
Outdated
], | ||
"web_accessible_resources": [ | ||
"inpage.js" | ||
"inpage.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 don't think we want to make all resources web-accessible. This seems like a potential privacy breech.
I think you should specifically list the files needed to display the page statuses (html, images).
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.
Sure, I have removed it.
app/scripts/lib/ipfsContent.js
Outdated
|
||
resolver.resolve(name, provider).then(ipfsHash => { | ||
clearTimeout(clearTime) | ||
let url = 'https://gateway.ipfs.io/ipfs/' + ipfsHash |
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.
Are we sure we want to hit IPFS gateway and not Infura?
Sending this comment to Infura for comment.
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.
Sure, I have changed to the ipfs.infura.io
function getProvider (type) { | ||
switch (type) { | ||
case 'mainnet': | ||
return 'https://mainnet.infura.io/' |
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 would rather we use the user's provider, provided by MetaMask, to reduce our number of blockchain connections, but for this feature, I can see the benefit of this simplicity, there aren't other chains w/ the ENS resolver anyway, this could be optimized later...
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.
No problem, we will keep updating this part in the future.
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.
yo no estoy de acuerdo en la utilización de trezor, para poder retirar mi fondos
@danfinlay you can use https://ipfs.infura.io for this. We've put a lot of work into preparing for additional traffic for IPFS. |
@PhyrexTsai this would be very nicely accompanied by:
|
I've verified this works with When I got it working, at first it usually didn't work, so I think it maybe only works with new tabs after installation, not sure why it was intermittently not working on first uses. I mostly just want a guide on creating compatible sites, so others can build on this as soon as we release. |
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.
Works alright, has some issues with first time usage, and it doesn't save user's typed items, but does work with links like http://phyrextsai.eth, so that's cool, a blog post will really make this valuable to the community, but that doesn't need to block inclusion.
@danfinlay Sorry for the late reply, I have write a tutorial of how to post websites by using IPFS and interact with ENS. Tutorial link: https://gist.github.com/PhyrexTsai/cffcbfa1d752b9cf817d920dfcd1ec9f And also have some website already deployed |
@@ -154,6 +158,7 @@ async function initialize () { | |||
const initLangCode = await getFirstPreferredLangCode() | |||
await setupController(initState, initLangCode) | |||
log.debug('MetaMask initialization complete.') |
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.
@danfinlay For the first time can not connect with, the issue that the initState.NetworkController.provider
is undefined
, so I think we can have a default value for this issue.
The fix code look likes:
let defaultProvider = initState.NetworkController === undefined ? { type: "mainnet" } : initState.NetworkController.provider;
ipfsContent(defaultProvider);
The guide looks great @PhyrexTsai. Would you mind if we reposted that on our blog when it's time to publish? |
@danfinlay Sure, that will be great, thank you very much |
we're cutting a bug fix release tomorrow, then this will go in after with the next batch of larger features 😸 |
Any way to make this resolve to the IPFS content, rather than redirecting to it? |
@PhyrexTsai @danfinlay @kumavis thanks for creating/adding this very important feature. Can´t wait to move the oasis products to IPFS/ENS. |
@Arachnid does a plugin has the right to change DNS hosts and actually register a "eth" TLD, so it looks like a traditional address? Honestly it would be great, but I would understand if google didn't permitted it as it would make phishing powers of extensions much worse |
Shouldn't this feature use IPLD instead of IPFS? The |
Are pathnames forwarded? E.g. if I go to mywebsite.eth/home it should redirect to /home Event better would be to resolve IPFS content directly. Is this possible to do in a chrome extension @PhyrexTsai ? |
From what I’ve seen, you can resolve content directly if you register a new scheme. So like |
As @Georgi87 mentioned about pathnames on the domain like `http://phyrextsai.eth/home` will pass to IPFS hash and append `/home` on redirection url. You can try the example below: http://phyrextsai.eth/index.html http://phyrextsai.eth/images/phyrex.jpg
@danfinlay @Georgi87 I have committed the feature to let the mechanism in the pathnames of .eth domain working as the same in the ordinary domain. Example: |
A few files are not passing our linting rules. We can either:
The affected files are:
These errors can be seen here: Or by running: These may be automatically fixable by running |
@PhyrexTsai Please check PortalNetwork#4 If you merge that into your branch and push, all the test / lint errors should go away |
Lint fix for Integration ENS with IPFS
@danfinlay @brunobar79 Thanks for helping, I've merged the PR on this branch. |
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.
Let's do this!
can't wait for this 🚀 |
Note that not all ENS names will end in .eth. Can you make this work for other TLDs too? |
@Arachnid What are the planned TLD's or how will new TLD's be proposed and added? |
@bgits .luxe and .dds are likely to be added in the very near future, and correspond to their DNS equivalents. In the longer run, nearly any DNS TLD will be supported via DNSSEC proofs. |
"https://*.infura.io/", | ||
"activeTab", | ||
"webRequest", | ||
"*://*.eth/" |
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.
This seems to appear in permissions dialog for Chrome as "eth websites" without an indication it's talking about a TLD. Leaving the user wondering "What is an eth website?"
Simple Summary
This implementation is to build a way to resolves ENS domains (.eth) to IPFS hash and redirects to url content.
Motivation
MetaMask browser extension is great project helps users to interact with Ethereum, and furthermore can be more powerful by add content resolve by ENS.
Specification
Resolve from ENS Public Resolver
getContent
hash to IPFS hash, and redirect url toipfs.infura.io
.Demo
Video
https://www.dropbox.com/s/8g8y42z6w7e6bcj/metamask.mov?dl=0