-
Notifications
You must be signed in to change notification settings - Fork 2
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: IPFS retrieval client #243
Conversation
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
This reverts commit f74672b.
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
I did a test in Brave:
My conclusion is that IPFS Gateways and Brave's built-in IPFS node (presumably based on Kubo) has different default behaviour from Lassie:
If we feel this can be confusing to Zinnia users, then I am proposing to enforce modules to explicitly include either Having said that, considering the early alpha status of Zinnia, I think we can afford to introduce breaking changes, and it's better to invest our time in things adding more value. @juliangruber WDYT? |
Reminder for myself: after we land this PR, I need to update our Windows release builds to include golassie.dll in the zip archives. |
@@ -21,6 +21,8 @@ deno_fetch = "0.129.0" | |||
deno_url = "0.105.0" | |||
deno_web = "0.136.0" | |||
deno_webidl = "0.105.0" | |||
lassie = "0.3.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.
Neat. I didn't know there is a Rust client!
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's a thin Rust wrapper embedding the original Go Lassie, I started the project three weeks ago :)
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 happy with fetch('ipfs://bafycid') API? I believe this API is provided by Brave when the IPFS integration is enabled.
It's intuitive and already established, I'd also say let's keep it until we require a different strategy (which could be added in addition to the always nice fetch() API)
Do we want to hide any of the Lassie response headers from Station Modules making the retrieval requests?
I'm not sure whether keeping or removing has advantages. Let's see what happens?
cli/main.rs
Outdated
|
||
let lassie_daemon = Arc::new( | ||
lassie::Daemon::start(lassie::DaemonConfig { | ||
temp_dir: None, // TODO: Should we use something like ~/.cache/zinnia/lassie? |
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.
Station Core will pass $CACHE_ROOT
:
Can we make Zinnia use that if it is set?
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 am already using CACHE_ROOT in zinniad
, see here:
The file cli/main.js
implements zinnia
, the CLI people use locally when building Station modules. Let's explore together what a good developer experience would look like?
IMO:
- We should not force
zinnia
users to always provideCACHE_ROOT
. We don't ask them forFIL_WALLET_ADDRESS
either. This way, users can typezinnia run main.js
, and all works out of the box. - I guess allowing CLI users to control the CACHE ROOT can be helpful. I am not sure, though, if an env var provides good ergonomy. Would a project-specific config file be easier to use?
- How important is this? Can we leave the current solution and open a follow-up GH issue to discuss what would a good (and easy-to-implement) solution look like?
- Note: if we tell Lassie to use a specific temp dir that's not automatically cleaned by the operating system, we will need to clean any leftover files ourselves.
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.
Note: if we tell Lassie to use a specific temp dir that's not automatically cleaned by the operating system, we will need to clean any leftover files ourselves.
I'll be implementing this cleanup in zinnia
as part of #245
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 suggesting always needing to pass CACHE_ROOT
, I thought if it's not passed then lassie shall pick its own temp dir, if it is passed (as in Station Core) it shall use that, just to keep all of the files together.
The primary use case for changing lassie's temp dir to me is not CLI usage but inside Station.
Note: if we tell Lassie to use a specific temp dir that's not automatically cleaned by the operating system, we will need to clean any leftover files ourselves.
That's a great point! What does Lassie use by default rn? If it uses an OS cleaned up dir, I'd suggest to leave it at that and add a comment summarizing our discussion 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.
That's a great point! What does Lassie use by default rn? If it uses an OS cleaned up dir, I'd suggest to leave it at that and add a comment summarizing our discussion here
Quoting from Lassie comments:
https://github.com/filecoin-project/lassie/blob/afc2ee5a4bc6f5e22ef2cc69396cc9b25f57b854/pkg/lassie/lassie.go#L199-L201
// WithTempDir allows you to specify a custom temp directory for bitswap
// retrievals, used for a temporary block store for the preloader. The default
// is the system temp directory.
I think that should be good enough for now, even if we may end up leaving some temporary files behind when zinnia
exists unexpectedly.
runtime/js/fetch.js
Outdated
|
||
export function fetch(resource, options) { | ||
let request = new Request(resource, options); | ||
// Fortunately, Request#url is a string, not an instance of URL class |
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.
What is fortunate about that?
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.
Great question! (The original answer got lost in the git history.)
The fetch
API accepts a wide range of types for the "resource" argument. Quoting from https://developer.mozilla.org/en-US/docs/Web/API/fetch#parameters:
resource
This defines the resource that you wish to fetch. This can either be:
- A string or any other object with a stringifier — including a URL object — that provides the URL of the resource you want to fetch.
- A Request object.
If request#url
was preserving the original value, then I would need to figure out how our custom fetch
wrapper can detect an object with a stringier and call that stringier to obtain the resource URL as a string.
What's fortunate: the conversion from "resource in one of the many supported formats" to "resource URL as a string" is already handled by the Request constructor.
Can you suggest how to improve my code comment to make this matter easier to understand for future readers?
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.
Ah, gotcha!
What do you think about
// Fortunately Request#url is always a string, no matter what was used to construct it
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 decided to write a longer comment, see 7db4ee8
// The `resource` arg can be a string or any other object with a stringifier — including a URL
// object — that provides the URL of the resource you want to fetch; or a Request object.
// See https://developer.mozilla.org/en-US/docs/Web/API/fetch#parameters
// Fortunately, Request's constructor handles the conversions, and Request#url is always a string.
// See https://developer.mozilla.org/en-US/docs/Web/API/Request/url
I couldn't agree more! |
Co-authored-by: Julian Gruber <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
See space-meridian/roadmap#19
Discussion points:
fetch('ipfs://bafycid')
API? I believe this API is provided by Brave when the IPFS integration is enabled.Example use
Response object (body is a CAR stream):
TODO
fetch('ipfs://bafycid')
fetch(new URL('ipfs://bafycid'))
fetch(new Request(...))
Out of scope
zinniad
starts - moved to Delete Lassie temp dir whenzinniad
starts #245