feat(rpc, api): add gossipsub IPC methods #296
Conversation
This commit represents the state of n0-computer/iroh#669 before the repo changes.
nathanielc
force-pushed
the
expose-gossipsub
branch
from
f4a1743 to
34ce902
Compare
This change removes any changes that were not related to exposing gossipsub subscription events. Additionally it updates tests to pass using the new APIs.
nathanielc
force-pushed
the
expose-gossipsub
branch
from
47065f0 to
3d22ba0
Compare
|
|
||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub enum GossipsubEvent { | ||
| Subscribed { |
There was a problem hiding this comment.
In ipfs-embed we had subscribe return just the messages.
But I guess it can be useful to have the subscribe/unsubscribe events as well...
There was a problem hiding this comment.
I know some of the tests make use of the subscribe/unsubscribe messages and generally I agree it makes sense to expose them to consumers.
There was a problem hiding this comment.
Yes, you can always filter them out if you don't want them.
The topic was inside the both the message event and the message data. It now only lives within the message data section. This avoids ambiguity of knowing which topic to use and keeping them synchronized.
|
@rklaehn Thanks for the review. I have addressed all feedback. |
| enum PeerState { | ||
| Connected(ConnectionId), | ||
| Responsive(ConnectionId, ProtocolId), | ||
| Unresponsive, | ||
| #[default] |
There was a problem hiding this comment.
Just so I understand: this is not really related to the PR, right?
Keep it in, it seems reasonable...
There was a problem hiding this comment.
Correct, clippy complained about it so I fixed it. Not sure what changed with clippy to start complaining about it but figured it was a good fix anyway.
iroh-p2p/src/node.rs
Outdated
| use crate::{ | ||
| behaviour::{Event, NodeBehaviour}, | ||
| rpc::{self, RpcMessage}, | ||
| Config, | ||
| }; | ||
|
|
||
| #[allow(clippy::large_enum_variant)] | ||
| #[derive(Debug, Clone)] | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] |
There was a problem hiding this comment.
Why does this need to be serializable now? The new rpc message just serializes GossipsubEvent, not this.
There was a problem hiding this comment.
Correct as well, removed those bits and everything passes tests.
|
Looks like there is a flaky transfer test. I'll try and hunt it down and get a fix. |
|
Ok I have not been able to find the source of the flaky test 😞. However I did trace it down into the bitswap code session management. From what I can tell a session worker is somehow dropped and it stops polling the future and then no progress is made causing a deadlock. I currently don't have time to continue to chase this bug however I would still like to move forward with this PR as it does not appear to have introduced this bug. Thoughts? |
Closes #16
This change is based off n0-computer/iroh#669 and removes any logic from that PR not related to exposing the gossipsub IPC methods