-
Notifications
You must be signed in to change notification settings - Fork 23
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
Initial Public FS and WebAssembly Work #6
Conversation
2b1de45
to
ff1d167
Compare
a3d5a20
to
a48f72f
Compare
a4ec85f
to
8e1b90e
Compare
9acdc7d
to
2547c39
Compare
d4d94ee
to
c711c58
Compare
c711c58
to
16b07ba
Compare
/// For types that implement getting a block from a CID. | ||
#[async_trait(?Send)] | ||
pub trait BlockStoreLookup { | ||
async fn get_block<'a>(&'a self, cid: &Cid) -> Result<Cow<'a, Vec<u8>>>; |
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 may just be me learning rust:
Can we return a slice [u8]
here instead of a Vec<u8>
?
Why do we return something clone-on-write-able? I was thinking a user of get_block
would never want to change the result, so if there's no need for writes, we won't need the Cow
.
crates/fs/common/blockstore.rs
Outdated
#[async_trait(?Send)] | ||
pub trait BlockStoreCidLoad { | ||
/// Loads a decodable object from the store with provided CID. | ||
async fn load<T: Decode<C>, C: Codec>(&self, cid: &Cid, decoder: C) -> Result<T>; |
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 is cool!
I'm thinking it might be better if this were a normal function instead of a trait though.
Something akin to async fn load<T: Decode<C>, C: Codec, B: BlockStoreLookup>(blockStore: &B, cid: &Cid, decoder: c) -> Result<T>
This way we wouldn't have to re-implement load
for all the different structs that implement BlockStore
. They all should work the same.
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 are right. We don't need to make it a trait here.
|
||
/// A directory in a WNFS public file system. | ||
#[derive(Debug, Clone, PartialEq, Eq)] | ||
pub struct PublicDirectory(Shared<PublicDirectoryInner>); |
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 highly in favor of going with only Rc<...>
in here for now, instead of Rc<RefCell<...>>
, and remove mutation from our algorithms currently.
@appcypher and I talked about this, I think mutation is used in the upsert
function. It may be slightly more complex to do immutably, but it's definitely possible.
We need the immutability for some of our upcoming algorithms, namely the base-history-on stuff to construct previous links and the file tree merge algorithm. They like to early-return when they detect that the two trees they compare have the same reference. These early returns are important, since we don't want to recurse into the file tree deeply so we don't fetch deeper nodes.
|
||
/// Looks up a node by its path name in the current directory. | ||
/// | ||
/// TODO(appcypher): What is a valid path segment identifier? |
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.
Yeah, that's a great question. I can only link an old internal discussion about this: https://talk.fission.codes/t/valid-paths-in-wnfs/2015
My thinking is, let's assume UTF8. Technically any byte array is valid (even ones that utf8-decoded include a slash), because of how the encoding into IPLD works out.
This is something we'd need to think about a little harder if we want to implement the pretty tree maybe.
pub struct OpResult<T> { | ||
// The root node. It is the same as the previous root node if the directory has not been diverged. | ||
pub root_node: Shared<PublicNode>, | ||
// Implementation dependent but it usually the last leaf node operated on. | ||
pub result: T, | ||
/// Whether this is a divergence or not. | ||
pub diverged: bool, | ||
} |
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 think we might be able to go without the OpResult
type.
get_node
returns anOpResult
, but that means you get back aPublicNode
inroot_node
andresult
, where I'd pretty much expectroot_node
to be the same thing that that's used to callget_node
on.read
returns anOpResult
, but likeget_node
, it returns back theroot_node
you originally calledread
on.write
returns the parent of the written node. Thatresult
isn't used anywhere at the moment.
diverged
may not be needed when stuff is immutable, but I may just not really understand it yet! 😅
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.
First end-to-end public filesystem implementation! 🎉
Stephen and I talked about the comments I left above: The crux of it would be a change somewhat deeper down & technically only a refactor, the surface API would stay the same.
So let's capture that refactor (figuring out how/whether we can remove the RefCell
, and make all algorithms only make use of mutation locally without needing a refcell and non-recursive) in an issue and we'll tackle that as the next PR. 👍
Technically we can continue poking the wasm blob this generates 😄
Summary
This PR implements the following features
This is the first of a few PRs that will implement the WNFSv2 Public file system.
Test plan (required)
Testing the Rust core.
cargo test -p wnfs
Testing the wasm binding.
cd crates/wasm
wasm-pack test --chrome
Closing issues
Fixes #4
Ongoing Issues