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

feat: metadata protocol shard subscription #2149

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Oct 23, 2023

Description

The shards a node support can dynamically changes and the metadata protocol must be updated at run-time to reflect this.

Changes

  • Added shard subscription listener to metadata protocol
  • Refactor/clean-up

Tracking #1940

Fix #2105

@SionoiS SionoiS added the E:1.4: Sharded peer management and discovery See https://github.com/waku-org/pm/issues/67 for details label Oct 23, 2023
@SionoiS SionoiS self-assigned this Oct 23, 2023
@github-actions
Copy link

github-actions bot commented Oct 23, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2149

Built from accfeb7

@SionoiS SionoiS force-pushed the feat--metadata-protocol-shard-subscription branch 3 times, most recently from a1614cc to b0c3c2c Compare October 24, 2023 19:04
Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

really good, thanks for this.

  • minor comment regarding exception raising
  • can we add some units tests for this?

waku/waku_metadata/protocol.nim Show resolved Hide resolved
waku/waku_metadata/protocol.nim Show resolved Hide resolved
@SionoiS
Copy link
Contributor Author

SionoiS commented Oct 25, 2023

  • can we add some units tests for this?

For the subscription part? Sure!

@SionoiS SionoiS requested a review from alrevuelta October 25, 2023 13:25
Copy link
Contributor

@alrevuelta alrevuelta 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 requested a review from jm-clius October 26, 2023 08:18
@SionoiS SionoiS force-pushed the feat--metadata-protocol-shard-subscription branch 2 times, most recently from b9f2f14 to 994b9d4 Compare October 27, 2023 12:45
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.

LGTM! Thanks for it!
Just added a few nitpicks but not blocking ones :)

On the other hand, I realized that the metadata protocol is being mounted within the creation of the Waku node. Why is that needed @alrevuelta? I wonder if we could follow the standard approach that we are following in other protocols so that all is more consistent.

tests/test_waku_metadata.nim Show resolved Hide resolved
waku/node/waku_node.nim Outdated Show resolved Hide resolved
waku/waku_metadata/protocol.nim Show resolved Hide resolved
waku/waku_metadata/protocol.nim Outdated Show resolved Hide resolved
@SionoiS SionoiS force-pushed the feat--metadata-protocol-shard-subscription branch from 0501ad1 to cb1d31b Compare October 27, 2023 15:33
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.

Thanks! Good step forward.

@SionoiS SionoiS force-pushed the feat--metadata-protocol-shard-subscription branch from cb1d31b to 31ea8e3 Compare October 30, 2023 19:17
@SionoiS SionoiS force-pushed the feat--metadata-protocol-shard-subscription branch from 31ea8e3 to c19b92a Compare October 30, 2023 20:57
@SionoiS SionoiS merged commit bcf8e96 into master Oct 30, 2023
8 checks passed
@SionoiS SionoiS deleted the feat--metadata-protocol-shard-subscription branch October 30, 2023 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E:1.4: Sharded peer management and discovery See https://github.com/waku-org/pm/issues/67 for details
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: automatically update shards in waku metadata protocol
4 participants