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

chore: add peer manager config to builder #1816

Merged
merged 2 commits into from
Jun 23, 2023
Merged

Conversation

alrevuelta
Copy link
Contributor

@alrevuelta alrevuelta commented Jun 22, 2023

Description

  • Currently the withPeerManager() in the builder was not implemented nor used. This PR moves the switch creation from waku_node to the builder and simplifies WakuNode.new() having less parameters.
  • This is done because otherwise, withPeerManger can't be used since to build the PeerManager, we need the Switch, since its built inside.
  • This PR adds a new unused flag max-relay-peers to be used in future PRs.
  • Note that it doesnt introduce any breaking change, unless maxConnections is configured <50, but this is not the case in the fleets.
  • Prerequisite for feat: limit relay connections below max conns #1813

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor comments but it looks great to me!
Thanks!

waku/v2/node/builder.nim Outdated Show resolved Hide resolved
waku/v2/node/builder.nim Outdated Show resolved Hide resolved
waku/v2/node/peer_manager/peer_manager.nim Show resolved Hide resolved
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@alrevuelta alrevuelta merged commit 71c4ac1 into master Jun 23, 2023
13 checks passed
@alrevuelta alrevuelta deleted the add-maxrelay-builder branch June 23, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants