feat(store): allow using a specific node #2192
Conversation
size-limit report 📦
|
packages/tests/tests/connection-mananger/connection_state.spec.ts
Outdated
Show resolved
Hide resolved
| return peer; | ||
| } else { | ||
| // peer is of MultiaddrInput type | ||
| const ma = multiaddr(peer); |
There was a problem hiding this comment.
yes, the class implementation can throw in several cases, mostly parsing of the multiaddr passed to it; why do you ask?
danisharora099
force-pushed
the
feat/specific-node
branch
from
060e316 to
6b452a5
Compare
packages/sdk/src/waku/waku.ts
Outdated
| @@ -143,9 +152,8 @@ export class WakuNode implements IWaku { | |||
| public async dial( | |||
| peer: PeerId | MultiaddrInput, | |||
| protocols?: Protocols[] | |||
| ): Promise<Stream> { | |||
| ): Promise<void> { | |||
There was a problem hiding this comment.
I totally missed this breaking change last time. Let's not introduce it.
There was a problem hiding this comment.
The reason why you are introducing it here is because connectionManager function is used, which is a good approach. But there it seems connectionManager.dial should be rethought as it provides additional features - such as attempt control etc. This is not needed in the context of public dial as I think.
Let's discuss if you think that multiple attempts should be done for public dial (i.e application developer trying to dial their node). If you think so, then I would advice to rewrite connectionManager.dial in following way
this way includes: making iterative approach and on success returning Stream.
There was a problem hiding this comment.
I'd prefer to proxy all dials through the ConnectionManager to keep things streamlined, but it's a catch-22 because StreamManager is created when are protocols are initialised, and for protocols to be initialised ConnectionManager is required
My reasoning behind this change was that Stream is never used from the public dial function, it is simply something returned by libp2p.dialProtocol is why it was part of the response type on the public dial function.
From the ConnectionManager.dialPeer(), we can return Stream | Connection type if we want, because libp2p.dial() returns a Connection while libp2p.dialProtocol returns Stream and both can be executed within ConnectionManager.dialPeer() based on if a bootstrap peer was provided to that function.
tl;dr:
- we can continue to return
voidfrom the publicdialfunction -- it's either a success or returns an error - return
Stream | Connectionfrom thedialfunction - return specifically
Streamby callinglibp2p.dialProtocolinstead of usingConnectionManagerfor it
IMO, any of the first two are ok, maybe 2nd option more because why not return something if we can, but I can't think of a usecase where a Stream response from a pubic dial would be useful
Streams are any way managed by the StreaManager
There was a problem hiding this comment.
we can continue to return void from the public dial function
it is not continue, void is getting introduced here
and tho I agree Stream, based on my experience, is barely used outside - I am not sure now and would like to avoid this change before concluding at least some investigation. If you did any research around, please, let me know, I'd prefer void
return Stream | Connection from the dial function
the thing is that you don't even need to create a fork in connectionManager.dial and you can use just dialProtocol with empty protocol list
https://github.com/libp2p/js-libp2p/blob/31a15a1483e0af0f9ede24de0a7f1d24bf9d408d/packages/libp2p/src/libp2p.ts#L287
so for now, I think it is better to:
- have
connectionManager.dialuse onlylibp2p.dialProtocol; - have
waku.dialbe as it was before - as we don't need retry guarantees on this function yet (can be introduced separately after discussion);
There was a problem hiding this comment.
have connectionManager.dial use only libp2p.dialProtocol;
We wanted to use ConnectionManager to dial (#2192 (comment)), thus moving the dial logic to ConnectionManager.dialPeer() meant that we internally use both methods conditionally: libp2p.dial() and libp2p.dialProtocol :
dialResponse = protocolCodecs
? await this.libp2p.dialProtocol(peerDialInfo, protocolCodecs)
: await this.libp2p.dial(peerDialInfo);
If protocolCodecs are provided, we want to use libp2p.dialProtocols() which returns us Stream, while if no protocolCodecs are specified, we use libp2p.dial() which returns us Connection
This is part of the larger ConnectionManager.dialPeer()
There was a problem hiding this comment.
have waku.dial be as it was before - as we don't need retry guarantees on this function yet (can be introduced separately after discussion);
If we do this, then we can continue to provide the same API response, but IMO it's best to proxy all dials from one function. Why would we not want features like retry attempts, keep alive management not part of the dial?
There was a problem hiding this comment.
How does this look? 7ac75e9
There was a problem hiding this comment.
created an issue : #2236
danisharora099
force-pushed
the
feat/specific-node
branch
2 times, most recently
from
84fed8f to
6a09ab1
Compare
danisharora099
force-pushed
the
feat/specific-node
branch
from
a4f0a3b to
0361cde
Compare
| * - Updates the peer store and connection state after successful/failed attempts | ||
| * - If all dial attempts fail, triggers DNS discovery as a fallback | ||
| */ | ||
| public async dialPeer(peer: PeerId | MultiaddrInput): Promise<Connection> { |
There was a problem hiding this comment.
it is not needed to be public
There was a problem hiding this comment.
leaving it public for now, would be interesting to experiment with this
Problem
Users need the ability to specify the peers used for their apps, for different protocols. Specifically for Store: https://discord.com/channels/1110799176264056863/1290694275683582085/1291644836742565929
Solution
Provides the ability to pass a specific node multiaddr while node creation to be used for Store. Falls back to random nodes if not passed.
Notes
Contribution checklist:
!in title if breaks public API;