-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Add IPFS hash of source files to metadata. #6599
Conversation
@@ -0,0 +1,360 @@ | |||
/* |
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 file was already part of this repository at some point :)
There was an error when running
Please check that your changes are working as intended. |
There was an error when running
Please check that your changes are working as intended. |
Did you see this https://github.com/axic/offline-ipfs/blob/master/index.js ? |
Ah nice! I couldn't find anything similar to your |
Codecov Report
@@ Coverage Diff @@
## develop #6599 +/- ##
==========================================
Coverage ? 87.02%
==========================================
Files ? 412
Lines ? 40059
Branches ? 4730
==========================================
Hits ? 34862
Misses ? 3628
Partials ? 1569
|
There was an error when running
Please check that your changes are working as intended. |
There was an error when running
Please check that your changes are working as intended. |
There was an error when running
Please check that your changes are working as intended. |
There was an error when running
Please check that your changes are working as intended. |
{ | ||
if (ipfsUrlCached.empty()) | ||
if (scanner->source().size() < 1025 * 256) | ||
ipfsUrlCached = "ipfs://" + dev::ipfsHashBase58(scanner->source()); |
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 should decide which URL format to use. The IPFS project seems to prefer /ipfs/<hash>
, but I don't think this is usable here because we just have a list of URLs instead of a key-value mapping. I have seen ipfs://ipfs/<hash>
, but I'm not sure what the consensus is inside the ipfs project. @axic do you know more here?
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 canonical way seems to be /ipfs/hash/path
, where compatibility versions exist, for example ipfs:/ipfs/hash/path
They say use this everywhere you can!
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.
See below (there are 2 years between the two comments...)
{ | ||
if (ipfsUrlCached.empty()) | ||
if (scanner->source().size() < 1025 * 256) | ||
ipfsUrlCached = "dweb:/ipfs/" + dev::ipfsHashBase58(scanner->source()); |
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 believe this is the recommended way to denote IPFS URIs as per ipfs/specs#152 (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.
It depends on which "upgrade stage" we want to 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.
We cannot use /ipfs
because we need a scheme, and I think we can afford not using ipfs://
because this is just a json artefact and not a website.
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.
Ok
string const& CompilerStack::Source::ipfsUrl() const | ||
{ | ||
if (ipfsUrlCached.empty()) | ||
if (scanner->source().size() < 1025 * 256) |
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.
1025?
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.
Fixed.
No description provided.