feat(rpc, api): add gossipsub methods and expose the p2p network_events to the Api - #669
feat(rpc, api): add gossipsub methods and expose the p2p network_events to the Api#669ramfox wants to merge 705 commits into
gossipsub methods and expose the p2p network_events to the Api#669Conversation
This focuses on creating tests for the `iroh get` CLI command. The Iroh API is also improved along the way. This required more work in the underlying API to make it more testable. What's tested here is everything *except* the actual IPFS behavior -- the focus is on the behavior of the CLI and API and its interactions with the filesystem. ## trycmd tests In `iroh/tests/cmd` you can see quite a few `.trycmd` files. These describes the command-line interaction. Stuff behind `$ is a command, there's a special `? failed` indicator if the command is considered to have failed, and the output of the command is shown. New are the `.out` and `.in` directories with the same names as the `.trycmd` files. These describe the filesystem before the command runs (may be missing), and the filesystem after the command has run. This way we can describe the effects of the `iroh get` command - directories and files are supposed to be created. We can also test failure scenarios where we refuse to overwrite a directory that already exists. #269 tracks various test cases. ## `get_stream` The `get` high level CLI method has been removed from the mockable `Api` trait (read on to see where it went). Instead, a more low level but still useful API method `get_stream` has been added to the `Api` trait. This gets a stream of relative paths and `OutType`, describing the directories and files you can create. The big difference with what was there before is that it returns relative paths and doesn't calculate final destination paths -- that's up to the user of the API. ## No `async_trait` macro for `Api` trait The interactions between the `Api` trait, `mockall` macro and `async_trait` were getting so hairy I couldn't figure out how to express things anymore once I wanted to add `get_stream`. For my sanity and also to learn better how this really works underneath, I've rewritten the `Api` trait to describe itself explicitly in terms of `(Local)BoxFuture` and `(Local)_BoxStream`. It's more verbose but functionally equivalent and I could express what I wanted. ## `ApiExt` trait The `ApiExt` trait is a trait that implements the high level `get` command. It puts everything together: it handles various error conditions, accesses the stream and then writes the stream to the filesystem. It's basically `iroh get`. The `ApiExt` trait is solely intended to contain default trait methods, and is automatically available when the `Api` contract is fulfilled (if you `use` it). Factored out `save_get_stream` from `getadd.rs` to be solely concerned with turning a stream into files and directories on the files system. That makes it possible to test its behavior in isolation. ## test fixture The `get` test fixture now mocks `get_stream` and returns a fake stream made from a `Vec`. This defines the actual stuff that the CLI writes to disk. ## relative_path Now depend on the [`relative-path` crate](https://crates.io/crates/relative-path) because what the stream returns are clearly relative paths, and we want to force the user to do something with them before being able to actually write stuff.
* feat: range request support * cr * tests & fixes * fix etag range headers * rebase cleanup
…iroh` commands (#348) * docs: add full help text for `lookup`, `connect`, `get`, `p2p`, and `iroh` commands * move long descriptions into constants Co-authored-by: b5 <sparkle_pony_2000@qri.io>
The store does not use the RPC client at all. Remove it.
We used to use the same config for the service and server (aka the binary). This is confusing when creating configs to use with e.g. iroh-one, iroh-embed or iroh-share because some fields are not used. This splits off the config structs to avoid this problem, services now have a Config and binaries a ServerConfig. This allows creating the services standalone without all the baggage a server needs. While this isn't many fields yet, this will get worse as we add more features (this is split off from another PR where this seemed useful).
ac5ed16 to
f2f42fd
Compare
iroh-share should used iroh-embedgossipsub methods and expose the p2p network_events to the Api
f2f42fd to
7baba0c
Compare
This removes the use of mockall which is causing a lot of trouble with the cargo features intricacies it brings with it. A lot of the tooling struggles with these different versions of the structs which also behave differently in surprising ways. The tests affected by this will be converted into end-to-end tests as part of is now unused. This is possibly only temprorary and that might come back when the end-to-end tests are added.
flub
left a comment
There was a problem hiding this comment.
If you'd like to punt on the API musings I have that's fine. We don't have to have a perfect API straight away and are probably free to keep breaking it for a while.
| Ok(P2pApi::new(p2p_client)) | ||
| } | ||
|
|
||
| pub fn store(&self) -> Result<StoreApi> { |
There was a problem hiding this comment.
Even if relatively trivial, I really appreciate doc comments especially on things that end up in the public API of iroh-embed. E.g. this would be a good place to give a summary of what a the store is and why you might want to use it directly.
| } | ||
|
|
||
| /// The list of [`Multiaddr`] that the Iroh p2p node is listening on | ||
| pub async fn addrs(&self) -> Result<Vec<Multiaddr>> { |
There was a problem hiding this comment.
From similar APIs in e.g. hyper I'm mildly tempted to call this local_addrs. Though I don't mind this name either.
| } | ||
|
|
||
| /// Get a stream of [`NetworkEvent`]. | ||
| pub async fn network_events(&self) -> Result<BoxStream<'static, Result<NetworkEvent>>> { |
There was a problem hiding this comment.
I'd be tempted to wrap this return type in a newtype for the API purposes.
Add `api.store()` method as way to get `StoreApi` from the API. This exposes the `put` method, which is needed in `iroh-share`. also moves CLI specific `connect` method to `iroh` and adds more general `connect` method to `iroh-api`
7baba0c to
378c805
Compare
add, get, & gossipsub methods This will get refactored to not be gross.
378c805 to
8517e1d
Compare
9adc9c6 to
3f9bbc9
Compare
| fn gossipsub_subscribe(self, req: GossipsubSubscribeRequest) -> GossipsubEventStream { | ||
| async move { | ||
| self.gossipsub_subscribe_0(req) | ||
| .await | ||
| .expect("FIX THIS - NEEDS TO BE ABLE TO RETURN A RESULT") | ||
| } | ||
| .flatten_stream() | ||
| .boxed() | ||
| } |
There was a problem hiding this comment.
@rklaehn I'm coming into conflict with this method in two ways.
For context, a GossipsubEventStream is BoxStream<'static, Box<GossipsubSubscribeResponse>>
First question/problem: I can't figure out how to handle the possible error from gossipsub_subscribe_0 When I try to use ? instead of expect here, I keep running into
cannot use the `?` operator in an async block that returns `Pin<Box<dyn Stream<Item = Box<iroh_rpc_types::p2p::GossipsubSubscribeResponse>> + std::marker::Send>>`
| this function should return `Result` or `Option` to accept `?
And when I looked up how to solve this, I didn't understand the syntax that the solutions suggested. Would love some help!
The second conflict I'm running into is with my "preferred" return type. My "dream" scenario is that the p2p RpcClient's gossipsub_subscribe can have a return type of:
Result<impl Stream<Item=GossipsubEvent>>
But, it seems like I'm forced to return a Result<GossipsubEvent> Item, based on the expectations of server_streaming. This isn't critical, but It would be nice to streamline the API in this way.
There was a problem hiding this comment.
A server streaming interaction returns a stream, not a result of stream. So if you have a situation where you have a possible error on creation of the stream, but then no errors once the stream is created, you have no choice but to have the stream item be a Result. Otherwise the try_flatten_stream thing won't work.
So the result must be impl Stream<Item=Result<GossipsubEvent>>, or we must extend quic-rpc to support a result of stream. Since it is all just decorators and most of the critical work is in the transports, this should be possible. Just a bit of work, so the question is how strongly you feel about this.
Is it really the case that once you have subscribed there is nothing whatsoever that can go wrong? That is usually quite rare. What I saw often is to have Result<impl Stream<Item = Result<...>>, in which case I figured you might as well flatten it.
|
Hello! The code that was previously hosted in this repository has been moved to n0-computer/beetle. If you are still interested in getting your PR merged, please re-open your PR on n0-computer/beetle. Check out our blog post for more info on our new direction for |
This commit represents the state of n0-computer#669 before the repo changes.
This commit represents the state of n0-computer/iroh#669 before the repo changes.
closes n0-computer/beetle#16
Gets
iroh-apiready for use iniroh-share(viairoh-embed):p2p.network_events- exposes stream ofNetworkEventsp2p.gossipsub_subscribep2p.gossipsub_publishp2p.gossipsub_add_peersp2p.peer_id- the local peer idp2p.addrs- the addresses of the local noderesolver- exposes the resolverstore- exposes the store methods