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!: accept one filed for network configuration #2027

Open
vpavlin opened this issue Jun 2, 2024 · 5 comments
Open

feat!: accept one filed for network configuration #2027

vpavlin opened this issue Jun 2, 2024 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@vpavlin
Copy link
Member

vpavlin commented Jun 2, 2024

This is a bug report

Problem

When contentTopics is specified, options.shardInfo will get populated here https://github.com/waku-org/js-waku/blob/master/packages/sdk/src/utils/libp2p.ts#L98

If shardInfo is specified, pubsubTopics get rewritten https://github.com/waku-org/js-waku/blob/master/packages/sdk/src/utils/libp2p.ts#L105

Proposed Solutions

Either error out if both shardInfo and pubsubTopics is populated, or do not overwrite pubsubTopics if they are explicitly defined

@fryorcraken fryorcraken added this to Waku Jun 2, 2024
@vpavlin
Copy link
Member Author

vpavlin commented Jun 2, 2024

WDYT @weboko ?

@weboko weboko moved this to Triage in Waku Jun 3, 2024
@weboko
Copy link
Collaborator

weboko commented Jun 3, 2024

@vpavlin, what is the case for users to specify both shardInfo and pubsubTopics?

@vpavlin
Copy link
Member Author

vpavlin commented Jun 4, 2024

No idea:) I think the case is specifying contentTopic and pubsubTopic - maybe even unintentionally without fully understanding what they are doing?

Consider this with assumption js-waku works with TWN:

  1. I specify contentTopic and lean on TWN
  2. I decide to move to my own pubsub for arbitrary reason, so I simply add pubsubTopic assuming that is all I need
  3. Things break:)

The actual case is this change 5b03709 broke things that previously worked - i.e. contentTopic and pubsubTopic specified at once.

Regardless of what the use case for having both shardInfo and pubsubTopic js-waku should let users know they should not specify both - i.e. report the error - as I suggested in the description.

@weboko
Copy link
Collaborator

weboko commented Jun 27, 2024

I think this is something that is not clear and we need more feedback on it.
Overall I think node should be run either in TWN or on custom network, not both.

For action point for this task I think we should refine createNode options as they are confusing.

My approach would be to:

  • accept pusbTopic or shardInfo;
  • have it as one field only and remove others;

Slightly intervened with #2034

@weboko weboko changed the title Silently rewriting pubsubTopics feat!: accept one filed for network configuration Jun 27, 2024
@weboko weboko self-assigned this Jun 27, 2024
@weboko weboko moved this from Triage to In Progress in Waku Jun 27, 2024
@weboko weboko added the enhancement New feature or request label Jun 27, 2024
@weboko weboko moved this from In Progress to Code Review / QA in Waku Jul 3, 2024
@weboko
Copy link
Collaborator

weboko commented Jul 10, 2024

This solution will be to complicated especially before we deprecate pubsubTopic - #2027 (comment)

Adding simple throw for now - #2056

@weboko weboko moved this from Code Review / QA to In Progress in Waku Jul 16, 2024
@weboko weboko moved this from In Progress to Code Review / QA in Waku Jul 16, 2024
@weboko weboko moved this from Code Review / QA to Done in Waku Jul 19, 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

2 participants