-
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
Ipfs cid v1 base32 #7362
Ipfs cid v1 base32 #7362
Conversation
Awesome! Here's the unit test output from CircleCI: 1312 passing (10s)
2 failing
1) AdvancedTab Component
should render correctly when threeBoxFeatureFlag:
AssertionError [ERR_ASSERTION]: 10 == 9
+ expected - actual
-10
+9
at Context.equal (ui/app/pages/settings/advanced-tab/tests/advanced-tab-component.test.js:19:12)
2) AdvancedTab Container
should map state to props correctly:
AssertionError [ERR_ASSERTION]: { warning: null,
sendHexData: false,
advancedInlineGas: false,
showFiatInTestnets: false,
autoLogoutTimeLimit: 0,
t... deepEqual { warning: null,
sendHexData: false,
advancedInlineGas: false,
showFiatInTestnets: false,
autoLogoutTimeLimit: 0,
t...
+ expected - actual
{
"advancedInlineGas": false
"autoLogoutTimeLimit": 0
- "ipfsGateway": [undefined]
"sendHexData": false
"showFiatInTestnets": false
"threeBoxDisabled": false
"threeBoxSyncingAllowed": false
at Context.deepEqual (ui/app/pages/settings/advanced-tab/tests/advanced-tab-container.test.js:38:12) |
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've tested this branch and verified its behavior. It redirects to subdomained gateway links, allowing different "decentralized" sites to not collide with cookie storage.
This is an immediate improvement in the security of our ens/IPFS loading, so thank you very much! It's great to see the feature so nicely integrated into our settings page and the rest of our files, this is a very clean contribution.
There are still some improvements we could make, but they can be in a future change:
I believe we may be able to incorporate redirecting so that we could preserve the URL context (either ens://
or ipfs://
etc). This would also allow decentrally-hosted sites to persist cookies even after the gateway is changed.
I think that API may be webRequest.onBeforeRequest().
Anyways, just noting that so we can investigate it next.
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.
Oh sorry, before approving, I wanted to check if we'd been in contact with the ipfs gateway that's added here. Are they ready for our traffic?
It would also be nice if we could resolve |
Yeah, that API can definitely be used to accomplish what you're proposing. Just for context, after discussing this with @kumavis, I recommended that @pldespaigne pursue the CIDv1 gateway solution primarily. |
Makes sense to me. Just noting that API to make sure we capture it. I've opened a new issue for that extension of this. Only blocker left here to me is get the go-ahead from ipfs to use that gateway. |
Awaiting answer from IPFS: ipfs/infra#493 |
Edit: @pldespaigne, just see about that PR against |
Ehh, hold on to that thought an extra second. I don't think they were accounting for the fact that some of our users run full ethereum nodes, and would also configure IPFS nodes, so this eth-link solution is actually a hard turn for the unconfigurable, and a fair amount of work more. Let's take a little more time, maybe work with infura, to get an ipfs gateway that we can use as default, and possibly continue using this approach for now. |
just updated |
A little update, we have not found anyone who wants to freely host IPFS content in this format yet. We may need to explore an option where we use |
add ipfs gateway to advanced settings use ipfs gateway from settings
@rekmarks I've added one last comment, then this LGTM |
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.
Approving after review by @whymarrh and earlier review by @danfinlay
resolved out of band
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.
LGTM
Fixes #5724
This PR add the following features to MetaMask :
To test it you can :