chore: replace Option with Opt by fcecin · Pull Request #4035 · logos-messaging/logos-delivery

fcecin · 2026-07-14T12:32:52Z

Description

This PR refactors the code to use Opt in place of Option.

Some tricky bits in the port that you'll notice when reviewing this PR:

readValue overload for Opt[T] in tools/confutils/cli_args.nim (TOML; must live in the module that expands confutils' load);
StoreServiceConf loses {.requiresInit.}: Nim can't default-construct a requiresInit object holding a case-object field, and Opt[T] is one;
optionals now render ok(x) / none() instead of some(x) / none(T) in logs and --help (four sites where the new rendering was wrong, with e.g. an error printing as ok(…), are fixed at the source line as well);
The broker codec for Opt in persistency/backend_comm.nim needs three procs: broker Opt codec: marshal, unmarshal, and also size;
Syntax Opt.some() and Opt.none() was chosen; but ok() and err() can be used instead (but they look like error-handling code). Can't use naked some() and none() for multiple reasons (std/options collision is one of them). Canonical use in the ecosystem is Opt.some() and Opt.none() already;
Macros in presto and confutils literally use and thus require "Option," so I just kept those. Changing this requires fixing macros and makes this PR not only longer but more complex; we can do that in a follow-up issue/PR if that's actually necessary, so this PR remains large in line count but trivial to review;
This PR also rolls in a few, small compilation fixes for examples/ and apps/ that weren't compiling, since the Opt migration touched apps and examples. Harder for validation to split then fix separately than just include, so I fixed both issues here.

Changes

change all usage of std.options.Option[T] to results.Opt[T]
fix broken apps and examples (to validate refactor)
removed all std/options code added for libp2p v2 migration
add broker Opt codec to persistency/backend_comm.nim
add a readValue overload for Opt[T] in tools/confutils/cli_args.nim
keep Option where required (Presto, confutils' config_file.nim)

Issue

closes #3955

* change all usage of std.options.Option[T] to results.Opt[T] * fix broken apps and examples (to validate refactor) * removed all std/options code added for libp2p v2 migration * add broker Opt codec to persistency/backend_comm.nim * add a readValue overload for Opt[T] in tools/confutils/cli_args.nim * keep Option where required (Presto, confutils' config_file.nim)

github-actions · 2026-07-14T12:33:04Z

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 release-notes is added to make sure any changes to the user interface are properly announced in changelog and release notes.

github-actions · 2026-07-14T12:33:04Z

This PR may contain changes to database schema of one of the drivers.

If you are introducing any changes to the schema, make sure the upgrade from the latest release to this change passes without any errors/issues.

Please make sure the label release-notes is added to make sure upgrade instructions properly highlight this change.

github-actions · 2026-07-14T12:43:09Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:4035

Built from 1092b96

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

Ivansete-status

LGTM! Thanks for it! 💯
It sounds interesting to submit a separate PR to adjust the REST/presto part.
Cheers.

Ivansete-status · 2026-07-16T10:31:27Z

-      waitFor c.node.rln.generateRLNProof(message.toRLNSignal(), float64(time))
+      try:
+        waitFor c.node.rln.generateRLNProof(message.toRLNSignal(), float64(time))
+      except CatchableError:


I wonder if we could instead handle the possible exception inside generateRLNProof so that we have better granularity of where it occurred. And therefore we can annotate that proc with .async: (raises: []).

Ivansete-status · 2026-07-16T10:34:13Z

  if args.ethRpcEndpoint == "":
    # Create a basic configuration for the Waku node
    # No RLN as we don't have an ETH RPC Endpoint
-    conf.mode = Core


These changes seems aside from PR's purpose.

Yes I explained above, the examples don't compile.

Ivansete-status · 2026-07-16T10:37:56Z

 const discv5Port = 9000

-proc setupAndPublish(rng: ref HmacDrbgContext) {.async.} =
+proc setupAndPublish(rng: crypto.Rng) {.async.} =


I'm fine with that but seems unrelated?

Yes, the examples are broken. I'm fixing them here.

Ivansete-status · 2026-07-16T10:39:27Z

-import logos_delivery/waku/compat/option_valueor
-import std/[atomics, options, macros]
-import chronicles, chronos, chronos/threadsync, ffi
+import std/[atomics, macros], chronicles, chronos, chronos/threadsync, ffi


nit, even though we not always do, we traditionally separated imports in three categories:

std

explicit nimbledeps (former vendor deps.)

internal modules

Suggested change

import std/[atomics, macros], chronicles, chronos, chronos/threadsync, ffi

import std/[atomics, macros]

import chronicles, chronos, chronos/threadsync, ffi

Ivansete-status · 2026-07-16T10:51:49Z

+import
+  std/sets,
+  chronos,
+  web3,
+  stew/byteutils,
+  stint,
+  results,
+  chronicles,
+  logos_delivery/waku/incentivization/rpc,
+  tests/waku_rln_relay/utils_onchain


Suggested change

import

std/sets,

chronos,

web3,

stew/byteutils,

stint,

results,

chronicles,

logos_delivery/waku/incentivization/rpc,

tests/waku_rln_relay/utils_onchain

import

std/sets,

chronos,

web3,

stew/byteutils,

stint,

results

import

chronicles

import

logos_delivery/waku/incentivization/rpc,

tests/waku_rln_relay/utils_onchain

Ivansete-status · 2026-07-16T11:02:32Z

+  mixin mtMarshalSizeValue
+  result = sizeof(uint8)
+  if value.isSome():
+    result += mtMarshalSizeValue(value.get())


Let's avoid the use of implicit result value.

Ugh, classic Claude.

Besides the point, but I still want resurrect the Styler project sometime in the future. We've been using GenAI for a few months now, and I guess now I have better ideas about how to automate these recurrent style checks.

Ivansete-status · 2026-07-16T11:06:01Z

+  std/options,
+  std/[strformat, sugar],
+  results,
+  chronicles,
+  uri,
+  json_serialization,
+  presto/route


Suggested change

std/options,

std/[strformat, sugar],

results,

chronicles,

uri,

json_serialization,

presto/route

std/options,

std/[strformat, sugar],

results

import

chronicles,

uri,

json_serialization,

presto/route

Ivansete-status · 2026-07-16T11:06:41Z

-  chronicles,
-  chronos,
-  chronos/apps/http/httpserver
+  std/[strutils, net], regex, results, chronicles, chronos, chronos/apps/http/httpserver


Suggested change

std/[strutils, net], regex, results, chronicles, chronos, chronos/apps/http/httpserver

std/[strutils, net], regex, results

import chronicles, chronos, chronos/apps/http/httpserver

Ivansete-status · 2026-07-16T11:07:59Z

+import
+  chronicles,
+  eth/keys,
+  stew/[arrayops, endians2],
+  stint,
+  results,
+  ./rln_interface,
+  ../conversion_utils,
+  ../protocol_types,
+  ../protocol_metrics


Suggested change

import

chronicles,

eth/keys,

stew/[arrayops, endians2],

stint,

results,

./rln_interface,

../conversion_utils,

../protocol_types,

../protocol_metrics

import stew/[arrayops, endians2],

stint,

results

import

chronicles,

eth/keys

import

./rln_interface,

../conversion_utils,

../protocol_types,

../protocol_metrics

Ivansete-status · 2026-07-16T11:10:08Z

+import
+  results,
+  chronicles,
+  chronos,
+  metrics,
+  ./common,
+  ./rpc,
+  ./rpc_codec,
+  ../node/peer_manager


Suggested change

import

results,

chronicles,

chronos,

metrics,

./common,

./rpc,

./rpc_codec,

../node/peer_manager

import

results

import

chronicles,

chronos,

metrics

import

./common,

./rpc,

./rpc_codec,

../node/peer_manager

* Change generateRlnProof error handling * Fix imports * Remove result use

NagyZoltanPeter

My claude review found no serious issues with it. IMHO its better to get into master asap due to the wide blast of change radius.
Thanks for it.

Follow-up to the rebase onto master's repo-wide Option[T] -> Opt[T] change (#4035). Converts the code this branch adds to the new convention: - messaging/rest_api/types.nim: MessagingJsonEnvelope fields to Opt[T], Opt.some/Opt.none, and json_serialization/pkg/results instead of json_serialization/std/options. - tests: WakuNodeConf.clusterId is now Opt[uint16]; DTO fields are Opt. `Option[ContentBody]` in the handlers is presto's own API and stays as-is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…bility (#4014) * WIP logosdeliverynode app initial commit * WIP - extra cli option * WIP: messaging client REST endpoints * Add event poll for messaging rest with cache mechanism * Messaging rest tests * test: assert 404 via raw string client in messaging REST test presto's typed REST client raises RestDecodingError when it cannot decode a non-2xx text error body into the response type. Add a RestResponse[string] stub (messagingGetSendEventsByIdRawV1) and point the "already-polled id -> 404" assertion at it, matching the relay REST test pattern. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * remove customized cli args as confutils has no support for it * Introduce --entry-layer and re-introduce --mode flags into cli args, applied new driver into LogosDelivery + tests * Add messaging REST client test * Add docker image build of logosdeliverynode for CI builds * Fix tests * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Refactor Messaging REST API to better match Messaging Send and Receive APIs * chore: migrate messaging REST API to Opt[T] Follow-up to the rebase onto master's repo-wide Option[T] -> Opt[T] change (#4035). Converts the code this branch adds to the new convention: - messaging/rest_api/types.nim: MessagingJsonEnvelope fields to Opt[T], Opt.some/Opt.none, and json_serialization/pkg/results instead of json_serialization/std/options. - tests: WakuNodeConf.clusterId is now Opt[uint16]; DTO fields are Opt. `Option[ContentBody]` in the handlers is presto's own API and stays as-is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

github-actions Bot assigned fcecin Jul 14, 2026

fcecin requested review from Ivansete-status, NagyZoltanPeter, darshankabariya and stubbsta July 14, 2026 17:44

NagyZoltanPeter requested a review from Copilot July 14, 2026 23:04

Copilot AI reviewed Jul 14, 2026

Ivansete-status approved these changes Jul 16, 2026

View reviewed changes

fcecin added 3 commits July 16, 2026 10:24

Fixes from Ivan's review

5d24d41

* Change generateRlnProof error handling * Fix imports * Remove result use

Follow literal import grouping suggestion

463d58a

Merge remote-tracking branch 'origin/master' into chore/migrate-opt

5d1c883

fcecin mentioned this pull request Jul 16, 2026

chore: replace minprotobuf with nim-protobuf-serialization #4046

Open

NagyZoltanPeter approved these changes Jul 16, 2026

View reviewed changes

fcecin merged commit ce918b0 into master Jul 16, 2026
14 checks passed

fcecin deleted the chore/migrate-opt branch July 16, 2026 17:02

	import std/[atomics, macros], chronicles, chronos, chronos/threadsync, ffi
	import std/[atomics, macros]
	import chronicles, chronos, chronos/threadsync, ffi

	std/[strutils, net], regex, results, chronicles, chronos, chronos/apps/http/httpserver
	std/[strutils, net], regex, results
	import chronicles, chronos, chronos/apps/http/httpserver

Uh oh!

Conversation

fcecin commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Issue

Uh oh!

github-actions Bot commented Jul 14, 2026

Uh oh!

github-actions Bot commented Jul 14, 2026

Uh oh!

github-actions Bot commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Ivansete-status left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NagyZoltanPeter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fcecin commented Jul 14, 2026 •

edited

Loading

github-actions Bot commented Jul 14, 2026 •

edited

Loading