-
Notifications
You must be signed in to change notification settings - Fork 8
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: use @helia/verified-fetch #63
Conversation
src/heliaServer.ts
Outdated
if (isDnsLabel(address)) { | ||
address = dnsLinkLabelDecoder(address) | ||
} |
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.
@SgtPooki small nit: (unless we check before) we should check if address
is a valid CID (with ipns name) first, and apply this normalization only if it is not a valid CID – see some prior art in https://github.com/ipfs/ipfs-companion/blob/7ca6433418909d43ec8801f27e417514614a164c/add-on/src/lib/ipfs-path.js#L71
(better safe than sorry, there could be a multibase encoding which uses .
and -
)
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.
since helia-verified-fetch handles this entirely now, we should be able to remove this code
@@ -54,6 +54,7 @@ $ docker run -it -p 8080:8080 -e DEBUG="helia-http-gateway*" helia-http-gateway: | |||
| `TRUSTLESS_GATEWAYS` | Comma separated list of trusted gateways to fetch content from | [Defined in Helia](https://github.com/ipfs/helia/blob/main/packages/helia/src/block-brokers/trustless-gateway/index.ts) | | |||
| `USE_LIBP2P` | Whether to use libp2p networking | `true` | |
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 wonder if we couldn't just merge the USE_BITSWAP
and USE_LIBP2P
vars to just be use @helia/http
or use helia
?
Like, USE_HTTP_GATEWAYS_ONLY
or something to select @helia/http
.
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'm down with finagling the configs in another PR once we get verified-fetch in
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.
moved to #65
@SgtPooki what's the status of this PR? It'd be great to land it to test the integration of things like bitswap sessions, etc. |
@achingbrain I need to update to the latest verified-fetch. most of my focus has been on service-worker-gateway, but I can get this up to date |
playwright test is failing for drand. so i removed it. see probe-lab/tiros#11 for more details |
while (!done) { | ||
const { done: _done, value } = await reader.read() | ||
if (value != null) { | ||
reply.raw.write(Buffer.from(value)) |
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 ignores backpressure. reply.raw.write
returns a boolean
value - if it's false we should wait for the stream to emit the 'drain'
event before continuing to write to it.
This PR is a start on the work required to adopt the new @helia/verified-fetch library currently in PR at ipfs/helia#392
Tasks