feat: incorporate noosphere_car enhancements #6
Conversation
| @@ -1,4 +1,3 @@ | |||
| /target | |||
| /iroh_gateway/test_files/* | |||
| .env | |||
| Cargo.lock | |||
There was a problem hiding this comment.
I'm adding Cargo.lock b/c the WASM CI job depends on this file existing.
There was a problem hiding this comment.
That seems odd, we can just generate it in CI
src/varint.rs
Outdated
| @@ -0,0 +1,89 @@ | |||
| //! This module has been adapted from the [integer_encoding] crate. The | |||
There was a problem hiding this comment.
Did you try asking them to entirely remove the Send bound? I don't think it is needed
There was a problem hiding this comment.
Confirmed: Send bound not required. I think we had added the conditional bound to avoid removing this one in the upstream implementation but I'm not sure.
There was a problem hiding this comment.
Ahh, yah, I think the integer-encoding crate enforces a Send bound that can't be met under Wasm. That's why we forked this module.
There was a problem hiding this comment.
Did you try asking them
If by "them" you mean integer-encoding maintainers, then: no! We didn't try. We should. Someone should do that. It would be the right thing to do....
😅
There was a problem hiding this comment.
Ahh, yah, I think the
integer-encodingcrate enforces aSendbound that can't be met under Wasm. That's why we forked this module.
Yeah that's likely because of async_traits Send requirements by default. But using https://docs.rs/async-trait/latest/async_trait/#non-threadsafe-futures it should be possible to avoid that.
There was a problem hiding this comment.
does a git branch ref prevent us from publishing an updated version of this crate?
It does. It is currently unclear if integer-encoding is still maintained unfortunately. @flub you reviewed various varint implementations, any suggestions to what we could switch?
There was a problem hiding this comment.
I mostly compared to a multiformat's varint with a dead simple re-implementation of QUIC's varint format (which was faster). Sadly I don't think any crate tidily exports QUIC's varint but it's super simple to write by hand: https://github.com/n0-computer/varint-bench/blob/main/src/lib.rs
There was a problem hiding this comment.
well we need to use the speced format, which I believe is the one from multiformats unfortunately.
There was a problem hiding this comment.
Humble suggestion: you can file an issue to address this in the future and commit the change as-is. In the mean time if there are any problems with it, you can always throw your hands up and rightly claim it was my choice to partially fork integer-encoding.
Alternatively: you can publish your patched crate and use that.
There was a problem hiding this comment.
aaalright, found a much cleaner version, we now use unsigned-varint which is the same dependency that multihash and friends already use.
| #[cfg(target_arch = "wasm32")] | ||
| wasm_bindgen_test::wasm_bindgen_test_configure!(run_in_browser); | ||
|
|
||
| const TEST_V1_CAR: &[u8] = include_bytes!("testv1.car"); |
There was a problem hiding this comment.
Please add a comment why these are using include_bytes instead of reading from disk at runtime
|
✅ |
drops integer-encoding, avoiding the dep and using the same dep that multihash already uses
|
@cdata, what do you think, are you willing to approve this? I want to make sure this still fits @subconsciousnetwork's needs, and if so we can cut a release & set to work on making sure the @banyancomputer team's needs are met |
|
I just released this as |
Pulls in streaming support & WASM from noosphere-car
@dignifiedquire, this also incorporates the crate updates from #5. Adds a WASM test suite based on noosphere CI