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

Fix nim-waku config inconsistencies #543

Merged
merged 4 commits into from
May 11, 2021
Merged

Conversation

jm-clius
Copy link
Contributor

@jm-clius jm-clius commented May 10, 2021

Background

This PR partially addresses #540. It fixes some inconsistencies in the naming of Waku v2 config items. It is the first of three steps necessary to change CLI parameters without breaking existing deployments.

Step 1 (this PR):

Add new, consistent config items in such a way that we still support the current items. In other words, keep the old CLI parameters intact. Refactor existing field names by postfixing them with _depr. This will mark them for removal in Step 3.

Step 2:

Propagate changes to existing deployments:

  • open PR adding support for the new config items on prod and test fleets
  • inform external nim-waku clients, such as WalletConnect

Step 3:

Remove deprecated config items from the codebase.

Conventions followed

I've tried adhering to the following conventions:

  1. config items with similar semantics should follow similar naming conventions (e.g. persist-peers should look similar to persist-messages)
  2. where a config item is composed of more than one word, use camelCase for field names while hyphenating the CLI parameter
  3. parameters that take a "plural" argument should be in plural form (e.g. prefer staticnodes to staticnode)
  4. the field name and CLI parameter for each item should agree
  5. use consistent namespacing where possible (e.g. change log-metrics to metrics-logging to be consistent with other metrics config items)

Summary of changes

The following CLI parameters have changed. The old parameters will be deprecated and removed in Step 3:

For wakunode2, chat2 and chat2bridge

  • nodekey changed to node-key UPDATE: this would make it inconsistent with Waku v1. Reverted in 455edea
  • dbpath changed to db-path
  • peerpersist changed to persist-peers
  • rlnrelay changed to rln-relay
  • staticnode changed to staticnodes UPDATE: reverted in f4dd214. Reasoning here
  • log-metrics changed to metrics-logging

For wakubridge

All items overlapping with the above list, with the following additions

  • fleetv1 changed to fleet-v1
  • staticnodev1 changed to staticnode-v1
  • nodekeyv1 changed to node-key-v1
  • staticnodev2 changed to staticnode-v2
  • nodekeyv2 changed to node-key-v2

What has not changed?

The concepts staticnodes, filternode and storenode are such tight concepts that I did not consider them as composites of two words. In other words, the config parameters were not changed to filter-node, store-node or static-nodes.

@jm-clius jm-clius requested review from oskarth, EbubeUd and staheri14 May 10, 2021 15:40
@oskarth oskarth requested review from kdeme and jakubgs May 11, 2021 05:05
Copy link
Contributor

@oskarth oskarth left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, thanks! I'd like to hear what Nimbus people think though, as it'd probably be a good idea to have this be roughly consistent across nim-waku, nimbus, nim-libp2p etc

Requesting review from @kdeme as he looked into this before, but @zah or @dryajov might also have some thoughts here

Copy link
Contributor

@jakubgs jakubgs left a comment

Choose a reason for hiding this comment

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

I don't know much Nim, but the name changes make sense to me.


staticnodes* {.
desc: "Multiaddr of peer to directly connect with. Argument may be repeated"
name: "staticnodes" }: seq[string]
Copy link
Contributor

Choose a reason for hiding this comment

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

I use this singular typically because of the way it works in confutils. At CLI it expects the usage as such --staticnode:... --staticnode:... for multiple staticnodes.
The actual variable can be plural as it holds the full provided list in a seq.
So to me it makes more sense like this from a UX PoV. It would be different when the CLI would expect a comma separated list. I've seen some users making that error in the past actually, trying to provide the comma separated list when it was called --staticnodes

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good point. As long as --help clearly indicates that it can be used multiple times.

It would also match how flags like these are named in Nimbus:
https://github.com/status-im/nimbus-eth2/blob/e1a8049e/beacon_chain/conf.nim#L109-L111
https://github.com/status-im/nimbus-eth2/blob/e1a8049e/beacon_chain/conf.nim#L314-L316


staticnodesV1* {.
desc: "Enode URL to directly connect with. Argument may be repeated"
name: "staticnodes-v1" .}: seq[string]
Copy link
Contributor

Choose a reason for hiding this comment

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

idem above


staticnodesV2* {.
desc: "Multiaddr of peer to directly connect with. Argument may be repeated"
name: "staticnodes-v2" }: seq[string]
Copy link
Contributor

Choose a reason for hiding this comment

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

idem


staticnodes* {.
desc: "Peer multiaddr to directly connect with. Argument may be repeated."
name: "staticnodes" }: seq[string]
Copy link
Contributor

Choose a reason for hiding this comment

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

idem

@kdeme
Copy link
Contributor

kdeme commented May 11, 2021

I've tried adhering to the following conventions:

1. config items with similar semantics should follow similar naming conventions (e.g. `persist-peers` should look similar to `persist-messages`)

2. where a config item is composed of more than one word, use camelCase for field names while hyphenating the CLI parameter

3. parameters that take a "plural" argument should be in plural form (e.g. prefer `staticnodes` to `staticnode`)

4. the field name and CLI parameter for each item should agree

5. use consistent namespacing where possible (e.g. change `log-metrics` to `metrics-logging` to be consistent with other `metrics` config items)

I agree with most of this, as being cleaner & nicer looking. Except probably point 3., see review.

For reference, in nimbus-eth2 we are also mostly following these rules I think. But for 3. we also keep it in singular.
There is for example bootstrap-node and direct-peer, in singular. And yes, as you can notice we do have a hypen at bootstrap-node versus bootnode used here. I did originally use bootnode and staticnode for the same reasons that you mentioned here. No strong preference on either though.

I quickly checked nimbus-eth1, story is a bit different there as rule 2 is not really followed. This probably stems from taking over most cli arguments from geth.

@jm-clius
Copy link
Contributor Author

Thanks, @kdeme. Very useful comments. I also agree now re staticnode - probably better to maintain singular form to indicate usage. Have reverted this in 1db5125.

@jm-clius
Copy link
Contributor Author

Update: I've reverted changing nodekey to node-key in 455edea. Reasoning: changing to node-key would make it inconsistent with Waku v1, where a nodekey config item already exists. Similar to staticnode, filternode and storenode, nodekey can be considered a tight enough concept not to be hyphenated.

@jm-clius jm-clius merged commit 50f2235 into master May 11, 2021
@jm-clius jm-clius deleted the fix/config-inconsistencies branch May 11, 2021 15:07
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.

4 participants