-
Notifications
You must be signed in to change notification settings - Fork 66
chore!: separate internal and CLI configurations #3357
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
base: master
Are you sure you want to change the base?
Conversation
0d18e4f
to
b09f19b
Compare
wsExtAddress = some( | ||
ip4TcpEndPoint(extIp.get(), wsBindPort.get(DefaultWsBindPort)) & | ||
wsFlag(wssEnabled) | ||
) | ||
except CatchableError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we just catch that exception in one place in wider try-catch? Seems this function distracted with this exception handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll review that for a follow-up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like an improvement unrelated to my changes so I'll keep it for a follow-up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is getting awesome! Thanks so much for it! 💯
I'm just adding some nitpick comments that I hope you find useful.
Also, not approving yet as I guess there will be more changes in the future.
tests/factory/test_waku_conf.nim
Outdated
) == clusterConf.discv5BootstrapNodes | ||
|
||
if clusterConf.rlnRelay: | ||
assert conf.rlnRelayConf.isSome |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick comment: in "verbs" statements the use of parenthesis is the tendency
Only in "nouns" we avoid parenthesis, such as in len
assert conf.rlnRelayConf.isSome | |
assert conf.rlnRelayConf.isSome() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to follow this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do note that std/options doc do not use ()
so you may be at odd with the ecosystem: https://nim-lang.org/docs/options.html#isSome%2COption%5BT%5D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do note that std/options doc do not use
()
so you may be at odd with the ecosystem: https://nim-lang.org/docs/options.html#isSome%2COption%5BT%5D
ah wow , I didn't know that. Thanks for sharing!
We try to align with the following: https://hackmd.io/@vac/main/%2F5oIVOuQcSM2WKrnDLUPcpg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do note that std/options doc do not use
()
so you may be at odd with the ecosystem: nim-lang.org/docs/options.html#isSome%2COption[T]ah wow , I didn't know that. Thanks for sharing! We try to align with the following: hackmd.io/@vac/main/%2F5oIVOuQcSM2WKrnDLUPcpg
This is different coding than what was share to me by other members of the nwaku team: https://status-im.github.io/nim-style-guide/language.result.html
I suggest you decide on which one to use and make it obvious in the README
waku/factory/internal_config.nim
Outdated
dns4DomainName = conf.dns4DomainName.map( | ||
proc(dn: DomainName): string = | ||
dn.string | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we also change the dns4DomainName
init's parameter's type to DomainName
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not yet sure what is the best usage of the distinct
types.
It's best if the sub-protocol define its own distinct types from its own config object. But I don't think I'll do that in this PR. I think I will want to tidy up and have the "one config object per protocol, defined in the protocol module, and imported + nested in WakuConf
" approach done in a follow-up PR. When I do that, then we can review distinct type usatge.
waku/factory/networks_config.nim
Outdated
# TODO: this file should be called cluster_conf.nim | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of renaming the file I think is better to rename the type because we are used to that file/module name when looking for in on VScode
waku/factory/node_factory.nim
Outdated
@@ -86,17 +86,24 @@ proc initNode( | |||
else: | |||
peerStore.get() | |||
|
|||
let (secureKey, secureCert) = | |||
if conf.webSocketConf.isSome and conf.webSocketConf.get().secureConf.isSome: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if conf.webSocketConf.isSome and conf.webSocketConf.get().secureConf.isSome: | |
if conf.webSocketConf.isSome() and conf.webSocketConf.get().secureConf.isSome(): |
waku/factory/node_factory.nim
Outdated
@@ -86,17 +86,24 @@ proc initNode( | |||
else: | |||
peerStore.get() | |||
|
|||
let (secureKey, secureCert) = | |||
if conf.webSocketConf.isSome and conf.webSocketConf.get().secureConf.isSome: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if conf.webSocketConf.isSome and conf.webSocketConf.get().secureConf.isSome: | |
if conf.webSocketConf.isSome() and conf.webSocketConf.get().secureConf.isSome(): |
waku/factory/waku_conf.nim
Outdated
relayShardedPeerManagement*: bool | ||
relayServiceRatio*: string | ||
|
||
proc log*(conf: WakuConf) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is much better to have a more especific proc name. Having too generic proc names makes it more difficult to lookup in VSCode.
proc log*(conf: WakuConf) = | |
proc logWakuConf*(conf: WakuConf) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is much better to have a more especific proc name. Having too generic proc names makes it more difficult to lookup in VSCode.
that sucks (lookup in VSCode).
I am a big proponent of "don't repeat context" but I see your point. I'll think on how I can cope with it :)
b09f19b
to
63f0f20
Compare
One note on defaults. I think that the The first point means that most likely, the config builders should be near their config, in the module of the logic (eg rln relay), because this module is best to know/define good agnostic defaults. And then force setting the parameters that don't have a good generalized/agnostic default (ie, they can change widely from an edge node to a service node). cc @gabrielmer Finally, regarding But I wonder if a better approach would be either:
For (2) to happen, messaging API, and the work I am doing here, should useful so it's super easy to create a new binary, without having to copy-past |
You can find the image built from this PR at
Built from 9ee3d27 |
8c587c9
to
a3addba
Compare
rlnRelayTreePath* {. | ||
desc: "Path to the RLN merkle tree sled db (https://github.com/spacejam/sled)", | ||
defaultValue: "", | ||
name: "rln-relay-tree-path" | ||
.}: string | ||
|
||
rlnRelayBandwidthThreshold* {. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the one deleted were actually not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may only need to adjust the README in the following: https://github.com/vacp2p/rln-interep-contract
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For research dev testing, it could be workable - presumably a researcher developing e.g. a new protocol for lightpush incentivisation, would then have to create new binaries based on the existing edge and service specialised binaries? Regression testing could be more difficult, as we will constrain the use case to what is set up for that specific binary. What would the impact be on QA/DST - they might want to start testing new protocols in parallel (as with Waku Sync). Would they now need to run simulation frameworks for each of |
I might have a huge lack of context here, but I can share the PoV from DST. Nwaku CLI arguments is something that I talked with @Ivansete-status sometimes already.
Another example that come to my mind is
What do I want to say with all of this? I can see the point on (if I understood correctly) separate functionalities in different binaries, but I don't still know why, but this doesn't feel quire right to me. I guess it would depend on where you draw the line. To me, the current approach of nWaku looks OK, the problem that I see is how the information is delivered to the user, and the problems that there are in the argument themselves as in coupled behaviours. Of course, again, I might not have the entire context here, and my opinion could change a bit depending on the information that I have. Because indeed this topic is quite complicated on its own, and then there is Waku itself, that it is not precisely an easy project. Edit: Also, we have to take into account the importance of the previous points. Sadly, in Waku's case, we are not talking about minor optimization issues, like the node is a bit slower, consumes more RAM, or things like that. |
a3addba
to
aca1512
Compare
This PR may contain changes to configuration options of one of the apps. If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed. Please also make sure the label |
This is something I am doing internally (see However, the tooling
Yes, I have also thought the same , filter has a
Yep, I totally agree. I first fixed that by adding a
Absolutely, in this PR, I am chaing the name to
and
My opinion is that the While Waku nodes are adaptive, I believe we should provide discreet configurations that we expect 80% (80/20 rule) of users and developers to use. I believe this actually matters a lot for Vac-DST. Because at the end of the day, we want simulation to match what users have in their hands. Namely, either a js-waku based web app, or a status app. My intent is to provide several
These should end up defined in the Messaging API. These are the mode that must be used by Status app, hence libwaku/c-binding should use those modes instead of enabling individual protocols. For example
Now, when running simulations, this is what DST should be using. Because running a bandwidth simulation with only relay, is not really meaning full as Status Desktop also provide those services. So if we want to tell Status users that their bandwidth will average to X, then it needs to be done with filter and light push service enabled (as they are enabled in desktop) Now, WDYT? --
Looking at I am not too sure what do you mean regarding regression testing. The researcher's code should still be in nwaku codebase. It just changing the way it's consumed:
So I'd expect the tests to still run as expected. |
e051473
to
06444ad
Compare
Not sure why
|
ok confirmed, this also happens on |
running fine in nwaku compose:
|
Split `WakuNodeConfig` object for better separation of concerns and to introduce a tree-like structure to configuration.
06444ad
to
239a489
Compare
This comment was marked as resolved.
This comment was marked as resolved.
I don't think this would involve too much changes from our side. |
Sure? libwaku currently builds properly for me on latest master, at least locally. And the CI passed for that last commit, so looks weird - we should for sure look into it and take care before merging this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wooow amazing work! 😍 😍
Thanks so much!
We should double check about why the build fails and added a small remark, really elegant refactor!
Indeed, I tried again |
libwaku compilation failure is odd, but fixed |
dogfooding via nwaku-compose looks good. |
all green on commit 55ab0eb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for it! 🙌
Adding some more comments that I hope you find useful!
Not approving yet as there are some points that I don't fully agree :)
Also, I'm super impressed how fast you got the Nim skills 🥳
apps/wakunode2/wakunode2.nim
Outdated
of generateRlnKeystore: | ||
let conf = wakuNodeConf.toKeystoreGeneratorConf().valueOr: | ||
error "Configuration failed", error = error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error "Configuration failed", error = error | |
error "Configuration failed in toKeystoreGeneratorConf", error = error |
apps/wakunode2/wakunode2.nim
Outdated
doRlnKeystoreGenerator(conf) | ||
of inspectRlnDb: | ||
let conf = wakuNodeConf.toInspectRlnDbConf().valueOr: | ||
error "Configuration failed", error = error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though chronicles gives line number I like to be as much explicit as possible ;P
error "Configuration failed", error = error | |
error "Configuration toInspectRlnDbConf failed", error = error |
var waku = Waku.new(confCopy).valueOr: | ||
var restServer: WakuRestServerRef = nil | ||
|
||
if conf.restServerConf.isSome: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if conf.restServerConf.isSome: | |
if conf.restServerConf.isSome(): |
# TODO: severals parameters are only needed when it's dynamic | ||
# change the config to either nest or use enum/type variant so it's obvious | ||
# and then it can be set to `requiresInit` | ||
dynamic*: bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is better the old name because it is much easier to lookup in the code. I suggest adding at least rln
prefix to all params :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can search rln.*dynamic
no? The naming redundancy makes the code very verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not super comfy :) but not blocking anyways
name: "discv5-discovery" | ||
.}: bool | ||
.}: Option[bool] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't quite agree with that. I prefer not use of Option
in this case but I might be missing something :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Option
here allows you to use the twn
preset, and yet deactivate discv5 (eg you are running an edge node).
But I can see some counter arguments/alternative solutions:
- All waku networks use discv5. So
discv5Enabled
should not actually be part of the TWNClusterConf
wakunode2
is not an edge node, so when can assume this option would be barely used.
I am inclined to go with (1). Do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, 1 sounds good to me. Maybe when using the edge
preset option, the discv5-discovery
should be false
and, as you mention, in all service
preset options, the discv5 should be true
.
conf.clusterId, conf.networkConf, conf.discv5Conf, conf.webSocketConf, | ||
conf.wakuFlags, conf.dnsAddrsNameServers, conf.portsShift, clientId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer less parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I added a todo. this should be done as we reduce the number of parameters on Netconfig.init
.
I suggest we do it in a follow pr to limit the scope of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started to look into this.
I think we can overhaul NetConfig
to align it with `WakuConf. But the scale of change is such that I think it's wiser to just leave it as it is and tackle it in a follow up PR.
Ideally, NetConfig
should just be nested in WakuConf
. And that will solve this kind of problem because anything needed network conf, such as enr generation, can just get this new NetConfig
/NetworkConf
parameter.
error "Retrieving dynamic bootstrap nodes failed", | ||
error = dynamicBootstrapNodesRes.error | ||
continue | ||
if waku.conf.dnsDiscoveryConf.isSome: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if waku.conf.dnsDiscoveryConf.isSome: | |
if waku.conf.dnsDiscoveryConf.isSome(): |
@@ -387,10 +357,17 @@ proc startWaku*(waku: ptr Waku): Future[Result[void, string]] {.async.} = | |||
return err("Error in updateApp: " & $error) | |||
|
|||
## Discv5 | |||
if waku[].conf.discv5Discovery or waku[].conf.discv5Only: | |||
if conf.discv5Conf.isSome: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if conf.discv5Conf.isSome: | |
if conf.discv5Conf.isSome(): |
return ok() | ||
|
||
proc validateNoEmptyStrings(wakuConf: WakuConf): Result[void, string] = | ||
if wakuConf.networkConf.dns4DomainName.isSome and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if wakuConf.networkConf.dns4DomainName.isSome and | |
if wakuConf.networkConf.dns4DomainName.isSome() and |
There are some more in the rest of the proc
macro with(builderType: untyped, argName: untyped, argType: untyped) = | ||
result = generateWithProc(builderType, argName, argType, argType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid the use of macro's as much as possible.
I encourage to use the same approach being used in the following, which is simpler and more explicit:
Also, is a good opportunity to split this file in different modules :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid the use of macro's as much as possible.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
Macros cause huge difficulties when debugging. We experience that when dealing with web3
.
Besides, the suggested change breaks consistency with other builder-patterns, that have a more explicit "with..." procs. Using a too generic proc name, e.g,. with
, makes it more difficult to look up where it is being used.
@Ivansete-status can you please clarify the points that are blocking the approval? I will not be able to tackle everything before my break. |
IMO, the most important point is related to |
Description
Split
WakuNodeConfig
object for better separation of concerns and to introduce a tree-like structure to configuration.Notes:
WakuNodeConf
usage in c-bindings for examples1024
default, make it clearer whatcontent-topics
do, etc)uint
instead ofint
)preset
option #3346RlnRelayConf
so that it's clear what parameters are needed fordynamic
orstatic
modes--preset
innwaku-compose
Changes
Option
to avoid dummy defaults. Also, nesting configurations such asRlnRelayConf
so that parameters need to be set only if the parent service is enabled; later, it opens the option for each protocol to define their own configuration and then be importer byWakuConf
WakuConf
validation logic is better contained; also see (1) where each protocol defining their config parameters can define their own logic; currently, validation logic are scattered betweennode_factory.nim
,waku.nim
, etcWakuConfBuilder
becomes to stop gap shop to build a valid config. It is lenient/flexible (everything is an option). This will enableWakuNodeConf
to be translated to a config.WakuConfBuilder
can be used; no need to use the giantWakuNodeConf
anymore.WakuNodeConf
andWakuConf
can also simplify things. for examplediscv5Only
can be aWakuNodeConf
only property, that translates to the right parameters onWakuConf
(e.g.relay: false, store: false
, etc)edge mode
orservice mode
as per message API, as it will be easy to create an easy CLI/builder option for those, and convert it to the rightWakuConf
setup.WakuConfBuilder
will provide sane defaults, so that it's easy to build binaries using nwaku (see howchat2.nim
is cluttered with config irrelevant to chat). What does not have a default because dev has to choose (egclusterId
), should be provided by a preset/ClusterConf
How to test
wakunode2
with various CLI args and confirm expected behaviour.nwaku-compose
would work on TWN with this