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: configure protected topics via cli #1696

Merged
merged 1 commit into from
May 2, 2023

Conversation

alrevuelta
Copy link
Contributor

@alrevuelta alrevuelta commented Apr 19, 2023

Summary:

Example:

./build/wakunode2 \
--topics="/waku/2/topic-1/proto /waku/2/topic-2/proto"\
--protected-topic=/waku/2/topic-1/proto:0425b97b092867bf74532d96e50b6227450b1413078efd414929ff444ece0f9146e58d6ac785c13e76111841812d6ddd20c83c88a6a24815948896731410b0c70b \
--protected-topic=/waku/2/topic-2/proto:04e790854444ae882cc69b25ec7bd454e7bdb8d4b900a4add3eeb52cf62415d9d93a5c7eb3c9ea08e8e854d8b8ac26f5073aaac6fa68c69b8f078bbd48c2d5589d

Output:

NOT 2023-04-19 16:39:53.554+02:00 routing only signed traffic                topics="wakunode" tid=3718069 file=wakunode2.nim:452 protectedTopic=/waku/2/topic-2/proto publicKey=04e790854444ae882cc69b25ec7bd454e7bdb8d4b900a4add3eeb52cf62415d9d93a5c7eb3c9ea08e8e854d8b8ac26f5073aaac6fa68c69b8f078bbd48c2d5589d
NOT 2023-04-19 16:39:53.554+02:00 routing only signed traffic                topics="wakunode" tid=3718069 file=wakunode2.nim:452 protectedTopic=/waku/2/topic-1/proto publicKey=0425b97b092867bf74532d96e50b6227450b1413078efd414929ff444ece0f9146e58d6ac785c13e76111841812d6ddd20c83c88a6a24815948896731410b0c70b
  • Blocked by wakunode2 changes, wait before merging.
  • Move config to Application-level configuration

@alrevuelta alrevuelta force-pushed the configure-protected-topics branch 2 times, most recently from 507c2d5 to 5b5d266 Compare April 19, 2023 14:57
@alrevuelta alrevuelta requested review from LNSD, rymnc and jm-clius and removed request for LNSD and rymnc April 19, 2023 14:58
@LNSD
Copy link
Contributor

LNSD commented Apr 19, 2023

Would you mind waiting for wakunode2 feature branch to be merged to merge this PR? cc @jm-clius

@alrevuelta
Copy link
Contributor Author

@LNSD sure, let me know :)

@alrevuelta alrevuelta added the blocked This issue is blocked by some other work label Apr 19, 2023
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.

In general, LGTM. We should be able to merge the wakunode2 app changes early next week, after which this should be mergeable. Mind moving the config item to a separate section with a commented title such as Application-level configuration. This is because it configures only application behaviour and not strictly speaking the node.


let publicKey = secp256k1.SkPublicKey.fromHex(elements[1])
if publicKey.isErr:
raise newException(ConfigurationError, "Invalid public key")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we append the publicKey.error()at this point?

Comment on lines +558 to +563
proc readValue*(r: var TomlReader, value: var ProtectedTopic) {.raises: [SerializationError].} =
try:
value = parseCmdArg(ProtectedTopic, r.readValue(string))
except CatchableError:
raise newException(SerializationError, getCurrentExceptionMsg())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could reduce some code:

Suggested change
proc readValue*(r: var TomlReader, value: var ProtectedTopic) {.raises: [SerializationError].} =
try:
value = parseCmdArg(ProtectedTopic, r.readValue(string))
except CatchableError:
raise newException(SerializationError, getCurrentExceptionMsg())
proc readValue*(r: var TomlReader, value: var ProtectedTopic) {.raises: [ConfigurationError].} =
value = parseCmdArg(ProtectedTopic, r.readValue(string))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mm unsure how this works under the hood, so just mirroring what we currently have in other readValue function parsing other arguments.

@alrevuelta alrevuelta force-pushed the configure-protected-topics branch from 5b5d266 to 9055cf5 Compare May 2, 2023 08:21
@alrevuelta alrevuelta force-pushed the configure-protected-topics branch from 9055cf5 to cb2a892 Compare May 2, 2023 08:28
@alrevuelta alrevuelta requested a review from jm-clius May 2, 2023 08:32
@alrevuelta
Copy link
Contributor Author

@jm-clius with the wakunode2 changes merged, this should be ready to go. Move also the config to a "appication-level" seciton.

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.

LGTM, thanks!

@alrevuelta alrevuelta merged commit 16b4452 into master May 2, 2023
@alrevuelta alrevuelta deleted the configure-protected-topics branch May 2, 2023 09:45
@vpavlin vpavlin mentioned this pull request May 16, 2023
1 task
@fryorcraken fryorcraken added the E:Opt-in message signing See https://github.com/waku-org/pm/issues/20 for details label Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked This issue is blocked by some other work E:Opt-in message signing See https://github.com/waku-org/pm/issues/20 for details
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants