Skip to content
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

update connectionManager.dispatchEvent test to listen to correct events #1835

Closed
fbarbu15 opened this issue Feb 8, 2024 · 6 comments
Closed
Labels
enhancement New feature or request

Comments

@fbarbu15
Copy link
Collaborator

fbarbu15 commented Feb 8, 2024

This is a change request

Problem

Update test according to comment

@fbarbu15 fbarbu15 added the bug Something isn't working label Feb 8, 2024
@fryorcraken fryorcraken added this to Waku Feb 8, 2024
@weboko
Copy link
Collaborator

weboko commented Feb 8, 2024

That's a good catch. I think to simplify everything we should expose and use only libp2p's dispatcher so that in any place in code we can do:

waku.dispatch // <- internal only
waku.addEventListener

and it will be the same as

waku.libp2p.*

@weboko weboko moved this to Triage in Waku Feb 8, 2024
@weboko weboko added the enhancement New feature or request label Feb 8, 2024
@danisharora099
Copy link
Collaborator

@fbarbu15 can you elaborate?
What do you aim to achieve?

@fbarbu15
Copy link
Collaborator Author

fbarbu15 commented Mar 6, 2024

Sure, my aim was to increase test coverage for waku.connectionManager
One of the available methods for connectionManager is waku.connectionManager.dispatchEvent and it doesn't works as I would expect (like waku.libp2p.dispatchEvent). So if it doesn't work I guess we should either remove it or fix it

@chair28980 chair28980 moved this from Triage to Priority in Waku Mar 6, 2024
@danisharora099 danisharora099 moved this from Priority to In Progress in Waku Mar 7, 2024
@danisharora099
Copy link
Collaborator

danisharora099 commented Mar 7, 2024

@fbarbu15 upon investigating this, I realise that the test case added isn't the right use of the API, thus making this issue not be a bug but an intended behaviour.

Let me explain:
We add an event listener PeersByDiscoveryEvents.PEER_CONNECT_BOOTSTRAP on connectionManager and you dispatch a peer:connect event on connectionManager as well with bootstrap peer ID.
This is incorrect as the Connection Manager listens on libp2p events for peer:connect, and not its own's.

Let me draw two scenarios which would be right for this:

  1. Connection Manager dispatches and listens to EPeerByDiscoveryEvents
  2. You listen for EPeersByDiscoveryEvents.PEER_CONNECT_BOOTSTRAP and dispatch peer:connect even with the bootstrap peer ID through libp2p's event emitter (which ConnectionManager should decode and eventually dispatch EPeersByDiscoveryEvents.PEER_CONNECT_BOOTSTRAP

I propose to update this issue description (since this isn't a bug) and change it into an enhancement where we refactor the existing test case & add the above two cases instead.

Moving this to triage while we reach a conclusion & update the issue description.

@danisharora099 danisharora099 moved this from In Progress to Triage in Waku Mar 7, 2024
@fbarbu15
Copy link
Collaborator Author

fbarbu15 commented Mar 7, 2024

You are correct, now that you explained it, it makes total sense, thank you!
Will update the description and update the test cases.

@fbarbu15 fbarbu15 changed the title bug: dispatchEvent doesn't work when called via IConnectionManager ex waku.connectionManager.dispatchEvent update connectionManager.dispatchEvent test to listen to correct events Mar 7, 2024
@fbarbu15 fbarbu15 removed the bug Something isn't working label Mar 7, 2024
@danisharora099
Copy link
Collaborator

resolved by #1897

@github-project-automation github-project-automation bot moved this from Triage to Done in Waku Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

No branches or pull requests

3 participants