search

waku-org / nwaku

Waku node and protocol.
Other
184 stars 47 forks source link

feature/start-protocol-command #209

Closed decanus closed 3 years ago

decanus commented 3 years ago

closes #207

decanus commented 3 years ago

@oskarth @kdeme this works, but it seems a bit ugly. Any suggestions on cleanup?

oskarth commented 3 years ago

suggestions on clean up

I'd look at how NBC does it probably

decanus commented 3 years ago

I kinda had in mind not exposing this relay, filter and store (perhaps not the latter) directly to the user. Sounds awfully difficult and I think the WakuNode abstraction could take care of this?

@kdeme not sure what you are saying here.

kdeme commented 3 years ago

What I meant is that the application should probably have cli options in the trend of: --full-node:true or --content-topics-only:blabla (or --light-node:true if you want), --historical-node:false, etc. And then this could be mapped to the right protocols, which would be the role of the WakuNode abstraction imo. But ok, we haven't thought well about this yet, so I guess it is fine for testing for now.

decanus commented 3 years ago

@kdeme what do you think of the new flags?

oskarth commented 3 years ago

What I meant is that the application should probably have cli options in the trend of: --full-node:true or --content-topics-only:blabla (or --light-node:true if you want), --historical-node:false, etc. And then this could be mapped to the right protocols, which would be the role of the WakuNode abstraction imo. But ok, we haven't thought well about this yet, so I guess it is fine for testing for now.

Full node seems off to me. What does that mean for protocols? Not obvious. Content topics - boolean or list? For client or for receiver? Light node in what context? Historical node as server? On what topic?

What behaviour should be done for each of these aside from mounting the protocol?

All of this should be discussed at the problem formulation state imo.

kdeme commented 3 years ago

I was merely giving these as example to show that the current solution is not fine for the end user, not to actually copy over and use as is. Hence the "in the trend of". And I also stated that I'm fine with merging this as is, as we haven't resolved that issue.

oskarth commented 3 years ago

Yeah I understand, I didn't take your initial comment as verbatim, just responded once I saw them in the code.

decanus commented 3 years ago

ok so let me revert to the previous.

oskarth commented 3 years ago

What's the state of this PR?

decanus commented 3 years ago

@oskarth theres some weird issues with the PR, I am working on fixing them.

decanus commented 3 years ago

@oskarth fixed. ptal, tested with chat2.

decanus commented 3 years ago

@oskarth should we modify the docker file to start the clusternode as a wakunode2 running all 3 protocols?

oskarth commented 3 years ago

@oskarth should we modify the docker file to start the clusternode as a wakunode2 running all 3 protocols?

This is the current behaviour right? And yeah we should

oskarth commented 3 years ago

Why is this PR touching nim-libp2p etc? Is it missing a rebase? I pushed a recent bump of submodules so I don't think we need one in this one.

decanus commented 3 years ago

Alright @oskarth, docker has been addressed as have gitsubmodules and default option.

decanus commented 3 years ago

@kdeme ptal