-
Notifications
You must be signed in to change notification settings - Fork 14
Conversation
a29c6a8
to
125abd8
Compare
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.
Code changes look great, I am curious about how some of the organization changed (i.e. using 1 CID for all content), that said, the only feedback is there are plenty of commented-out code that could just be removed (so no big deal if no action is taken).
I was hoping to test it locally if possible before approving. Is there any easy way to do so? Like a demo front-end app or anything of that sort? At the very least, I wanted to test with cargo test
.
When I initially tried running cargo test
on the branch, but it failed at the build step:
Updating git repository `https://github.com/spruceid/ssi`
error: failed to get `did-ethr` as a dependency of package `kepler-lib v0.1.0 (/Users/kelseyrhoda/magic/spruce/kepler/lib)`
Caused by:
failed to load source for dependency `did-ethr`
Caused by:
Unable to update https://github.com/spruceid/ssi?branch=feat/ucan-ipld-version#e5cf96b7
Caused by:
object not found - no match for id (e5cf96b7efd899a080b8f2cc763fc39a434ce536); class=Odb (9); code=NotFound (-3)
Any idea why, or how I could test / build it locally? The build seems more important, I could understand if the test suites aren't ready or don't prove all that much. It isn't a big deal if there isn't an easy way to test locally, I can definitely approve, then open issues if I have issues later integrating with SSX Quest, but I am curious why my build is failing.
I'm happy to approve once the build error is understood (if not solved).
lib/Cargo.toml
Outdated
did-webkey = { default-features = false, git = "https://github.com/spruceid/ssi", branch = "main" } | ||
did-onion = { default-features = false, git = "https://github.com/spruceid/ssi", branch = "main" } | ||
did-ion = { default-features = false, git = "https://github.com/spruceid/ssi", branch = "main" } | ||
did-method-key = { default-features = false, git = "https://github.com/spruceid/ssi", branch = "feat/ucan-ipld-version" } |
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.
Ideally these would point to a crates.io release, but hey, Rebase doesn't either.
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.
currently did-ion
v0.2 isn't yet released, that's the only blocker for using crates.io, but I can remove the branch ref
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 sounds like a good idea for now given our current use-case
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.
did-ion
is blocked by an ownership issue. Hopefully it will get resolved soon.
@@ -23,6 +23,9 @@ jobs: | |||
run: | | |||
rustup target add wasm32-unknown-unknown | |||
|
|||
- name: Install Protoc | |||
uses: arduino/setup-protoc@v1 |
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.
Do we need protobuf? Probably not, then you can use something like
libipld = { version = "0.14", default-features = false, features = ["dag-cbor", "dag-json", "derive", "serde-codec"]}
to avoid needing it for compilation
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.
CI still fails after removing it, looks like this is waiting on this pr
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, well then do make sure the docker image can still build
lib/Cargo.toml
Outdated
did-webkey = { default-features = false, git = "https://github.com/spruceid/ssi", branch = "main" } | ||
did-onion = { default-features = false, git = "https://github.com/spruceid/ssi", branch = "main" } | ||
did-ion = { default-features = false, git = "https://github.com/spruceid/ssi", branch = "main" } | ||
did-method-key = { default-features = false, git = "https://github.com/spruceid/ssi", branch = "feat/ucan-ipld-version" } |
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.
You can use the published versions now
}; | ||
|
||
pub type Block = OBlock<DefaultParams>; | ||
pub type BlockStores = Either<S3BlockStore, FileSystemStore>; |
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 don't understand how this is better than an enum with all the variants of storage options
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.
Also, why use generics for ImmutableStore
when in the end (I assume) it's always the Either
enum?
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 is yeah usually gonna be that enum, I figured it would make it easier to implement alternatives if we/someone else wanted to, and easier to change the generic in Orbit
if someone wants to deploy or embed it in a different context. e.g. if someone wanted to run a node in wasm for the browser using browser storage they could implement HypotheticalBrowserSessionStore
and just change the generic for that environment to that and avoid potential complications of building for wasm with dependancies like s3 (although I couldn't concretely name any such complications). in short: probably overzealous abstraction
50a92f1
to
1ca47cb
Compare
The rust-ipfs project has been archived in favour of a new rust implementation. Simultaneously we desire to decouple the design from the orthodox IPFS architecture for several reasons:
rust-ipfs
This PR removes the
rust-ipfs
dependancy while retaining thelibp2p
andlibipld
dependancy. In the process it:rust-ipfs
ImmutableStore
andStoreConfig
traits, implemented for the local filesystem, AWS S3 andstorage::either::Either
Orbit
and other structsKVStore
, in favour of singular unbounded-size content blocksOrbit
s more ergonomic