POC: structured api interface

[NagyZoltanPeter]

Important notice

This PR is a draft and issued for early examination and for open discussions in the team.
Do expect changes, new commits!

UPDATE

While the aim is still initiate discussion about the approach for long term, the PR reached state where it is mature enough even to be merged (as soon ci is green)

Important this PR not intended to change any behavior, nor re-wire all and everything.
It introduces a concept that allows us greater flexibility for later - smaller steps - refactorings.
This PR does not change any FFI facing API, nor adding new. All code changes out of ./logos_delivery/api/ folder is just to match the new interface requirements - with minimalist approach.

Notes to the reviewers

Although the PR seems extensive and touching lot of stuff, please focus on the followings.
API types and interfaces:

logos_delivery/api/
- types.nim
- logos_delivery_interface.nim
- kernel_interface.nim
- messaging_client_interface.nim
- reliable_channel_manager_interface.nim

API implementation locations:

./logos_delivery/
- logos_delivery.nim
- waku/factory/waku.nim
- messaging/messaging_client.nim
- channels/reliable_channel_manager.nim  

• Other changes are mostly semantic to adapt to some new shapes.
• One noticiable change is the unifiaction of WakuMode - noMode enum removed and exercised via Option[WakuMode] to enable single WakuMode shape everywhere (cli and API)
• Some changes are simple done to enable compilation. (such as examples/cpp/waku/cpp - it was not platform independent before)
• You can completely ignode .claude/** changes - coming with use of gitnexus under ClaudeCode + nim_brokers_instructions.md - that is also coding agent usage guide (maybe interesting though).

Description

This PR aims to showcase a desired API shape that matches our new layering of logos-delivery.
This POC uses latest nim-brokers that allows write natural Nim API interfaces and implementation.
The currently pinned version is an arbitrary commit - not a fixed version - due to need of fixes in the library to support fully all API shapes.
It allows us to turn current existing structures to be consumed over the interfaces.
Comes with easy mock-able interface classes- even per single API request level.

Currently target make onelogosdelivery exists only to validate compilation from source ./logos_delivery.nim - the new library root. It can be removed later as it does not produce any useful binary today.

Changes

Based on the latest code restructuring, and proper nim library format, the new API is placed at ./logos_delivery/api
It introduces exacting layer matching interfaces:

logos_delivery/api/
- logos_delivery_interface.nim
- kernel_interface.nim
- messaging_client_interface.nim
- reliable_channel_manager_interface.nim

• because optimally we should introduce our API facing types this POC brings those types into

logos_delivery/api/
- types.nim

Implementation wiring:
Each interface is implemented in the right layer spot:

Interfac srouce	Interface class	Implementation class	Implemenation source
logos_delivery_interface.nim	LogosDeliveryInterface	LogosDelivery	logos_delivery/logos_delivery.nim
kernel_interface.nim	KernelInterface	Waku	logos_delivery/waku/factory/waku.nim
messaging_client_interface.nim	MessagingClientInterface	MessagingClient	logos_delivery/messaging/messaging_client.nim
reliable_channel_manager_interface.nim	ReliableChannelManagerInterface	ReliableChannelManager	logos_delivery/channels/reliable_channel_manager.nim

As a temporal solution the POC introduces a new target (make + nimble) => onelogosdelivey

It builds with simple: make onelogosdelivery =>
builds

./build/onelogosdelivery/
- liblogosdelivery.dylib/.so/.dll

NO FFI is practiced from nim-brokers!

• logosdelivery.cddl # CBOR API description
• logosdelivery.h # CBOR C-ABI interface
• logosdelivery.hpp # C++ wrapper
• logosdeliveryConfig.cmake. # CMake config for easy consume of the lib + wrapper
• logosdeliveryConfigVersion.cmake # # CMake config for easy consume of the lib + wrapper
• logosdelivery.py. # Python wrapper
• logosdelivery_rs/src/lib.rs. # Rust wrapper
• logosdelivery_go/logosdelivery.go # Golang wrapper

Example C++ API shape is like this:
logosdelivery.hpp

Helper:
If you want to examing all the nim-brokers' generated source files that makes all these possible
use SRCGEN=1 make onelogosdelivery
the nim source files will be placed under build/broker_debug per broker types

Changes to existing codebase

• API types are elevated where it's been possible (e.g. not defined in upstream lib like SDS).
• WakuMode - cli_args now use Option[WakuMode] instead of having a bit controversial noMode element.
• ReliableChannelManager and ReliableChannel do not use sendHandler but instead utilize the MessagingClientInterface to issue lower layer send API call.
• Some event has bad name tag ordering - changed to more natural.
• LogosDelivery implementation class is not the master / orchestrator class of the library (both for nim and ffi users)
  • if promotes some API functions that are general to the library (getNodeInfo, getAvailableConfigs)

Issue

closes #3956

[NagyZoltanPeter]

C++ single header API shape:

// Generated by nim-brokers CBOR FFI codegen — do not edit.
//
// Header-only C++ wrapper around the C ABI declared in `logosdelivery.h`.
// Requires C++20 and jsoncons + jsoncons_ext/cbor in the include path.
#ifndef LOGOSDELIVERY_HPP
#define LOGOSDELIVERY_HPP

#include "logosdelivery.h"

#include <jsoncons/json.hpp>
#include <jsoncons_ext/cbor/cbor.hpp>

#include <cstdint>
#include <cstring>
#include <functional>
#include <memory>
#include <optional>
#include <span>
#include <string>
#include <system_error>
#include <unordered_map>
#include <utility>
#include <vector>

namespace logosdelivery {

template <typename T>
class Result {
    std::optional<T> value_;
    std::string error_;
public:
    static Result<T> ok(T value) {
        Result<T> r;
        r.value_ = std::move(value);
        return r;
    }
    static Result<T> err(std::string message) {
        Result<T> r;
        r.error_ = std::move(message);
        return r;
    }
    bool isOk() const { return value_.has_value(); }
    bool isErr() const { return !value_.has_value(); }
    explicit operator bool() const { return isOk(); }
    const T& value() const { return *value_; }
    T& value() { return *value_; }
    const T& operator*() const { return *value_; }
    const T* operator->() const { return &*value_; }
    T&& take() { return std::move(*value_); }
    const std::string& error() const { return error_; }
};

template <>
class Result<void> {
    bool ok_ = true;
    std::string error_;
public:
    static Result<void> ok() {
        Result<void> r;
        r.ok_ = true;
        return r;
    }
    static Result<void> err(std::string message) {
        Result<void> r;
        r.ok_ = false;
        r.error_ = std::move(message);
        return r;
    }
    Result() = default;
    bool isOk() const { return ok_; }
    bool isErr() const { return !ok_; }
    explicit operator bool() const { return isOk(); }
    const std::string& error() const { return error_; }
};

struct Bytes : std::vector<uint8_t> {
  using std::vector<uint8_t>::vector;
  Bytes() = default;
  explicit Bytes(std::vector<uint8_t> v) : std::vector<uint8_t>(std::move(v)) {}
};

// ---- Enums ----

enum class PagingDirection : int32_t {
  BACKWARD = 0,
  FORWARD = 1,
};

enum class ConnectionStatus : int32_t {
  Disconnected = 0,
  PartiallyConnected = 1,
  Connected = 2,
};

enum class WakuMode : int32_t {
  Core = 0,
  Edge = 1,
};

enum class NodeInfoId : int32_t {
  Version = 0,
  Metrics = 1,
  MyMultiaddresses = 2,
  MyENR = 3,
  MyPeerId = 4,
  MyBoundPorts = 5,
  MyMixPubKey = 6,
};

// ---- Object payload structs ----

struct MessageSentEvent;
struct MessageErrorEvent;
struct MessagePropagatedEvent;
struct WakuMessage;
struct MessageReceivedEvent;
struct Subscribe;
struct Unsubscribe;
struct MessageEnvelope;
struct ChannelMessageReceivedEvent;
struct ChannelMessageSentEvent;
struct ChannelMessageErrorEvent;
struct CloseChannel;
struct ReceivedMessage;
struct StoreQueryResponse;
struct WakuMessageKeyValue;
struct StoreQueryRequest;
struct ConnectionStatusChangeEvent;
struct StartAsNode;
struct Shutdown;
struct Stop;

// ---- Distinct / alias types ----

using RequestId = std::string;
using ContentTopic = std::string;
using Timestamp = int64_t;
using ChannelId = std::string;
using SdsParticipantID = std::string;
using PubsubTopic = std::string;
using RelayConnectedPeers = std::vector<std::string>;
using RelayPeersInMesh = std::vector<std::string>;
using WakuMessageHash = std::vector<uint8_t>;
using PeerIdsFromPeerstore = std::vector<std::string>;
using ConnectedPeersInfo = std::vector<std::string>;
using ConnectedPeers = std::vector<std::string>;
using PeerIdsByProtocol = std::vector<std::string>;
using DnsDiscovery = std::vector<std::string>;
using ListenAddresses = std::vector<std::string>;

struct MessageSentEvent {
  RequestId requestId{};
  std::string messageHash{};
};

struct MessageErrorEvent {
  RequestId requestId{};
  std::string messageHash{};
  std::string error{};
};

struct MessagePropagatedEvent {
  RequestId requestId{};
  std::string messageHash{};
};

struct WakuMessage {
  Bytes payload{};
  ContentTopic contentTopic{};
  Bytes meta{};
  uint32_t version{};
  Timestamp timestamp{};
  bool ephemeral{};
  Bytes proof{};
};

struct MessageReceivedEvent {
  std::string messageHash{};
  WakuMessage message{};
};

struct Subscribe {
};

struct Unsubscribe {
};

struct MessageEnvelope {
  ContentTopic contentTopic{};
  Bytes payload{};
  bool ephemeral{};
  Bytes meta{};
};

struct ChannelMessageReceivedEvent {
  ChannelId channelId{};
  SdsParticipantID senderId{};
  Bytes payload{};
};

struct ChannelMessageSentEvent {
  ChannelId channelId{};
  RequestId requestId{};
};

struct ChannelMessageErrorEvent {
  ChannelId channelId{};
  RequestId requestId{};
  std::string error{};
};

struct CloseChannel {
};

struct ReceivedMessage {
  PubsubTopic pubsubTopic{};
  WakuMessage message{};
};

struct StoreQueryResponse {
  std::string requestId{};
  uint32_t statusCode{};
  std::string statusDesc{};
  std::vector<WakuMessageKeyValue> messages{};
  std::optional<WakuMessageHash> paginationCursor{};
};

struct WakuMessageKeyValue {
  WakuMessageHash messageHash{};
  std::optional<WakuMessage> message{};
  std::optional<PubsubTopic> pubsubTopic{};
};

struct StoreQueryRequest {
  std::string requestId{};
  bool includeData{};
  std::optional<PubsubTopic> pubsubTopic{};
  std::vector<ContentTopic> contentTopics{};
  std::optional<Timestamp> startTime{};
  std::optional<Timestamp> endTime{};
  std::vector<WakuMessageHash> messageHashes{};
  std::optional<WakuMessageHash> paginationCursor{};
  PagingDirection paginationForward{};
  std::optional<uint64_t> paginationLimit{};
};

struct ConnectionStatusChangeEvent {
  ConnectionStatus connectionStatus{};
};

class MessagingClientInterface;
class ReliableChannelManagerInterface;
class KernelInterface;

struct __InstanceCtxEnvelope {
  std::optional<uint64_t> ok;
  std::optional<std::string> err;
};

namespace detail {
template <typename Owner, typename Traits>
class EventDispatcher;

struct MessageSentEventEventTraits;
struct MessageErrorEventEventTraits;
struct MessagePropagatedEventEventTraits;
struct MessageReceivedEventEventTraits;
struct ChannelMessageReceivedEventEventTraits;
struct ChannelMessageSentEventEventTraits;
struct ChannelMessageErrorEventEventTraits;
struct ReceivedMessageEventTraits;
struct ConnectionStatusChangeEventEventTraits;
} // namespace detail

class Logosdelivery {
 public:
  Logosdelivery();
  ~Logosdelivery();
  Logosdelivery(const Logosdelivery&) = delete;
  Logosdelivery& operator=(const Logosdelivery&) = delete;
  Logosdelivery(Logosdelivery&&) = delete;
  Logosdelivery& operator=(Logosdelivery&&) = delete;

  static std::string_view version() noexcept;

  Result<void> createContext();
  bool validContext() const noexcept;
  explicit operator bool() const noexcept;
  void shutdown() noexcept;
  uint32_t ctx() const noexcept;

  Result<void> startAsNode(std::string config);
  Result<std::unique_ptr<MessagingClientInterface>> startAsClient(WakuMode mode, std::string preset);
  Result<void> shutdown();
  Result<void> stop();
  Result<std::unique_ptr<KernelInterface>> kernel();
  Result<std::unique_ptr<MessagingClientInterface>> messaging();
  Result<std::unique_ptr<ReliableChannelManagerInterface>> channels();
  Result<std::string> getNodeInfo(NodeInfoId id);
  Result<std::string> getAvailableConfigs();

  using ConnectionStatusChangeEventCallback = std::function<void(Logosdelivery&, ConnectionStatus connectionStatus)>;
  uint64_t onConnectionStatusChangeEvent(ConnectionStatusChangeEventCallback fn) noexcept;
  void offConnectionStatusChangeEvent(uint64_t handle = 0) noexcept;

  std::string listApis();
  std::string getSchema();

 private:
  uint32_t ctx_ = 0;
  using ConnectionStatusChangeEventDispatcher = detail::EventDispatcher<Logosdelivery, detail::ConnectionStatusChangeEventEventTraits>;
  std::unique_ptr<ConnectionStatusChangeEventDispatcher> connection_status_change_eventDispatcher_;
};

// ---- MessagingClientInterface — sub-instance wrapper of MessagingClientInterface ----
class MessagingClientInterface {
 private:
  uint32_t ctx_ = 0;
  using MessageSentEventDispatcher = detail::EventDispatcher<MessagingClientInterface, detail::MessageSentEventEventTraits>;
  std::unique_ptr<MessageSentEventDispatcher> message_sent_eventDispatcher_;
  using MessageErrorEventDispatcher = detail::EventDispatcher<MessagingClientInterface, detail::MessageErrorEventEventTraits>;
  std::unique_ptr<MessageErrorEventDispatcher> message_error_eventDispatcher_;
  using MessagePropagatedEventDispatcher = detail::EventDispatcher<MessagingClientInterface, detail::MessagePropagatedEventEventTraits>;
  std::unique_ptr<MessagePropagatedEventDispatcher> message_propagated_eventDispatcher_;
  using MessageReceivedEventDispatcher = detail::EventDispatcher<MessagingClientInterface, detail::MessageReceivedEventEventTraits>;
  std::unique_ptr<MessageReceivedEventDispatcher> message_received_eventDispatcher_;

 public:
  explicit MessagingClientInterface(uint32_t ctx) noexcept;
  ~MessagingClientInterface();
  MessagingClientInterface(const MessagingClientInterface&) = delete;
  MessagingClientInterface& operator=(const MessagingClientInterface&) = delete;
  MessagingClientInterface(MessagingClientInterface&&) = delete;
  MessagingClientInterface& operator=(MessagingClientInterface&&) = delete;
  uint32_t ctx() const noexcept { return ctx_; }
  bool valid() const noexcept { return ctx_ != 0; }
  explicit operator bool() const noexcept { return ctx_ != 0; }
  void close() noexcept;
  Result<void> subscribe(ContentTopic contentTopic);
  Result<void> unsubscribe(ContentTopic contentTopic);
  Result<RequestId> send(MessageEnvelope envelope);
  using MessageSentEventCallback = std::function<void(MessagingClientInterface&, RequestId requestId, std::string_view messageHash)>;
  uint64_t onMessageSentEvent(MessageSentEventCallback fn) noexcept;
  void offMessageSentEvent(uint64_t handle = 0) noexcept;
  using MessageErrorEventCallback = std::function<void(MessagingClientInterface&, RequestId requestId, std::string_view messageHash, std::string_view error)>;
  uint64_t onMessageErrorEvent(MessageErrorEventCallback fn) noexcept;
  void offMessageErrorEvent(uint64_t handle = 0) noexcept;
  using MessagePropagatedEventCallback = std::function<void(MessagingClientInterface&, RequestId requestId, std::string_view messageHash)>;
  uint64_t onMessagePropagatedEvent(MessagePropagatedEventCallback fn) noexcept;
  void offMessagePropagatedEvent(uint64_t handle = 0) noexcept;
  using MessageReceivedEventCallback = std::function<void(MessagingClientInterface&, std::string_view messageHash, const WakuMessage& message)>;
  uint64_t onMessageReceivedEvent(MessageReceivedEventCallback fn) noexcept;
  void offMessageReceivedEvent(uint64_t handle = 0) noexcept;
};

// ---- ReliableChannelManagerInterface — sub-instance wrapper of ReliableChannelManagerInterface ----
class ReliableChannelManagerInterface {
 private:
  uint32_t ctx_ = 0;
  using ChannelMessageReceivedEventDispatcher = detail::EventDispatcher<ReliableChannelManagerInterface, detail::ChannelMessageReceivedEventEventTraits>;
  std::unique_ptr<ChannelMessageReceivedEventDispatcher> channel_message_received_eventDispatcher_;
  using ChannelMessageSentEventDispatcher = detail::EventDispatcher<ReliableChannelManagerInterface, detail::ChannelMessageSentEventEventTraits>;
  std::unique_ptr<ChannelMessageSentEventDispatcher> channel_message_sent_eventDispatcher_;
  using ChannelMessageErrorEventDispatcher = detail::EventDispatcher<ReliableChannelManagerInterface, detail::ChannelMessageErrorEventEventTraits>;
  std::unique_ptr<ChannelMessageErrorEventDispatcher> channel_message_error_eventDispatcher_;

 public:
  explicit ReliableChannelManagerInterface(uint32_t ctx) noexcept;
  ~ReliableChannelManagerInterface();
  ReliableChannelManagerInterface(const ReliableChannelManagerInterface&) = delete;
  ReliableChannelManagerInterface& operator=(const ReliableChannelManagerInterface&) = delete;
  ReliableChannelManagerInterface(ReliableChannelManagerInterface&&) = delete;
  ReliableChannelManagerInterface& operator=(ReliableChannelManagerInterface&&) = delete;
  uint32_t ctx() const noexcept { return ctx_; }
  bool valid() const noexcept { return ctx_ != 0; }
  explicit operator bool() const noexcept { return ctx_ != 0; }
  void close() noexcept;
  Result<ChannelId> createReliableChannel(ChannelId channelId, ContentTopic contentTopic, SdsParticipantID senderId);
  Result<void> closeChannel(ChannelId channelId);
  Result<RequestId> sendOnChannel(ChannelId channelId, Bytes payload, bool ephemeral);
  using ChannelMessageReceivedEventCallback = std::function<void(ReliableChannelManagerInterface&, ChannelId channelId, SdsParticipantID senderId, std::span<const uint8_t> payload)>;
  uint64_t onChannelMessageReceivedEvent(ChannelMessageReceivedEventCallback fn) noexcept;
  void offChannelMessageReceivedEvent(uint64_t handle = 0) noexcept;
  using ChannelMessageSentEventCallback = std::function<void(ReliableChannelManagerInterface&, ChannelId channelId, RequestId requestId)>;
  uint64_t onChannelMessageSentEvent(ChannelMessageSentEventCallback fn) noexcept;
  void offChannelMessageSentEvent(uint64_t handle = 0) noexcept;
  using ChannelMessageErrorEventCallback = std::function<void(ReliableChannelManagerInterface&, ChannelId channelId, RequestId requestId, std::string_view error)>;
  uint64_t onChannelMessageErrorEvent(ChannelMessageErrorEventCallback fn) noexcept;
  void offChannelMessageErrorEvent(uint64_t handle = 0) noexcept;
};

// ---- KernelInterface — sub-instance wrapper of KernelInterface ----
class KernelInterface {
 private:
  uint32_t ctx_ = 0;
  using ReceivedMessageDispatcher = detail::EventDispatcher<KernelInterface, detail::ReceivedMessageEventTraits>;
  std::unique_ptr<ReceivedMessageDispatcher> received_messageDispatcher_;

 public:
  explicit KernelInterface(uint32_t ctx) noexcept;
  ~KernelInterface();
  KernelInterface(const KernelInterface&) = delete;
  KernelInterface& operator=(const KernelInterface&) = delete;
  KernelInterface(KernelInterface&&) = delete;
  KernelInterface& operator=(KernelInterface&&) = delete;
  uint32_t ctx() const noexcept { return ctx_; }
  bool valid() const noexcept { return ctx_ != 0; }
  explicit operator bool() const noexcept { return ctx_ != 0; }
  void close() noexcept;
  Result<ContentTopic> buildContentTopic(std::string appName, uint32_t appVersion, std::string name, std::string encoding);
  Result<PubsubTopic> buildPubsubTopic(std::string topicName);
  Result<PubsubTopic> defaultPubsubTopic();
  Result<int64_t> relayPublish(PubsubTopic pubsubTopic, WakuMessage message, uint32_t timeoutMs);
  Result<bool> relaySubscribe(PubsubTopic pubsubTopic);
  Result<bool> relayUnsubscribe(PubsubTopic pubsubTopic);
  Result<bool> relayAddProtectedShard(uint16_t clusterId, uint16_t shardId, std::string publicKey);
  Result<RelayConnectedPeers> relayConnectedPeers(PubsubTopic pubsubTopic);
  Result<RelayPeersInMesh> relayPeersInMesh(PubsubTopic pubsubTopic);
  Result<bool> filterSubscribe(std::optional<PubsubTopic> pubsubTopic, std::vector<ContentTopic> contentTopics, std::string peer);
  Result<bool> filterUnsubscribe(std::optional<PubsubTopic> pubsubTopic, std::vector<ContentTopic> contentTopics, std::string peer);
  Result<bool> filterUnsubscribeAll(std::string peer);
  Result<std::string> lightpushPublish(PubsubTopic pubsubTopic, WakuMessage message, std::string peer);
  Result<StoreQueryResponse> storeQuery(StoreQueryRequest request, std::string peer, int64_t timeoutMs);
  Result<bool> connect(std::vector<std::string> peers, uint32_t timeoutMs);
  Result<bool> disconnectPeerById(std::string peerId);
  Result<bool> disconnectAllPeers();
  Result<bool> dialPeer(std::string peerAddr, std::string protocol, int64_t timeoutMs);
  Result<bool> dialPeerById(std::string peerId, std::string protocol, int64_t timeoutMs);
  Result<PeerIdsFromPeerstore> peerIdsFromPeerstore();
  Result<ConnectedPeersInfo> connectedPeersInfo();
  Result<ConnectedPeers> connectedPeers();
  Result<PeerIdsByProtocol> peerIdsByProtocol(std::string protocol);
  Result<DnsDiscovery> dnsDiscovery(std::string enrTreeUrl, std::string nameServer, int64_t timeoutMs);
  Result<bool> discv5UpdateBootnodes(std::vector<std::string> bootnodes);
  Result<bool> startDiscv5();
  Result<bool> stopDiscv5();
  Result<int64_t> peerExchangeRequest(uint64_t numPeers);
  Result<std::string> version();
  Result<ListenAddresses> listenAddresses();
  Result<std::string> myEnr();
  Result<std::string> myPeerId();
  Result<std::string> metrics();
  Result<bool> isOnline();
  Result<int64_t> pingPeer(std::string peerAddr, int64_t timeoutMs);
  using ReceivedMessageCallback = std::function<void(KernelInterface&, PubsubTopic pubsubTopic, const WakuMessage& message)>;
  uint64_t onReceivedMessage(ReceivedMessageCallback fn) noexcept;
  void offReceivedMessage(uint64_t handle = 0) noexcept;
};

[Ivansete-status]

Thanks for the PR!
Can we leave the ffi work aside for now?
We need to focus on internal layering only.
Let's have PRs of 1000 lines max and ideally 400 delta.

[Ivansete-status]

I prefer having that WakuMessage to remain at this level.

[fcecin]

Each layer will have to publish all types that it needs to speak with any upper layers. WakuMessage is a layer API type ("message" types usually are as they end up as arguments to the API procs).

In the C/FFI domain, we have complete control of what types actually make it out to users so we can hide WakuMessage if we want to.

We could try to have some kind of distinction in the Nim side e.g. differentiate "Layer interface" (procs, types) vs. "Nim library interface." We can achieve that by having a global nim "api" folder somewhere that imports selectively from the individual Nim layer API folders -- that ends up being the official API for "Nim Logos Delivery."

[Ivansete-status]

Let's add all these nexus files in a separate PR.

[Ivansete-status]

I see that API tries to flag this macro as FFI boundary.
I thought we mentioned that we would leave the ffi work aside for now and only focus on internal layering.

[fcecin]

An idea would be to at least pilot Broker FFI for the Kernel C/FFI surface, since that is the one we own completely and is, at least de jure, unsupported.

Another idea is to just allow this in, since we are not forced to use it as a back-end for the library. It documents what the FFI upgrade would look like from both Nim and C side. We can then run experiments where we wire the C/FFI surface to either ffi provider and run tests.

[fcecin]

Let's have PRs of 1000 lines max and ideally 400 delta.

For this PR in particular: it is impossible to size-constrain a refactor of this type and scope. I tried; it's impossible. This is not a routine maintenance or your everyday feature; this is a massive architectural jump that is already stripped to its barest form. If we land this PR (and we should, absolutely) we won't see another one like this for several years.

[fcecin]

This PR is fundamental and we have to prioritize it and merge it ASAP. It just needs (a) dedicated tests (start/stop tests, config tests, ...), and (b) any downstream fixes from test writing.

We can sort the config situation as we move forward. For now, it is fine to keep the single WakuNodeConf, the hardcoded channels prototype confs per channel, the p2pReliability flag for messaging api, etc.

[fcecin]

This bare var conf: WakuNodeConf I think needs, unfortunately, to be (something like):

var conf = defaultWakuNodeConf().valueOr:
  return err("Failed creating default conf: " & error)

For the same reasion tools/confutils/conf_from_json.nim currently has to do it (var conf = ?defaultWakuNodeConf()) -- that's in master (with my PR that extracted the config JSON parser). But in this PR, you can find the same behavior in liblogosdelivery/logos_delivery_api/node_api.nim:

registerReqFFI(CreateNodeRequest, ctx: ptr FFIContext[Waku]):
  proc(configJson: cstring): Future[Result[string, string]] {.async.} =
    ## Parse the JSON configuration using fieldPairs approach (WakuNodeConf)
    var conf = defaultWakuNodeConf().valueOr:
      return err("Failed creating default conf: " & error)

Apps do the same right now: examples/api_example/api_example.nim does var conf = defaultWakuNodeConf() then sets mode/preset.

We will for sure continue to refactor the config code (to e.g. truly centralize config defaults somewhere in definitive that's globally accessible). I won't submit any more config refactor PRs before this PR lands, since the API shape is what's going to constrain and to point to what we have to do, config-wise. For now we have to go through the whole pipeline as it is, mixing layers of "cli"-coded stuff, builder, etc., to make sure we concentrate and honor the scattered defaults as much as possible.

[NagyZoltanPeter]

Thanks it's a good catch. Yes this is just mimic of current state of config and open for change.

[fcecin]

The line above this is demanding self.waku to be nil, so this self.waku.node.started is a guaranteed nil deref on <nil>.node.

[NagyZoltanPeter]

You're right, the latter condition will be removed.

[Ivansete-status]

Some more comments/questions. Thanks!

[Ivansete-status]

This file is also only aiming for ffi purposes, and is not invoked internally by other Nim module. This can go in a separate discussion/PR. Besides, we already have nim-ffi for that purpose :)

[NagyZoltanPeter]

Yepp, let's discuss this separately. As said FFI exercise here is also a validation point as it really compiles to some meaningful. It's only API that enables FFI together with registerBrokerLibrary macro and compile time flag -d:BrokerFfiApi, that's it. Without it works as native Nim nim-brokers.

[Ivansete-status]

I might be missing something but this polymophism cost seems not needed.
I believe the intent is to make it mockable for testing but the way how the base interface is declared, logos_delivery/api/reliable_channel_manager_interface.nim, seems not idiomatic, i.e., I'd expect seeing either:

• base-annotated procs
• "concept" technique ( even though we never used that .)
• Use of generics

[NagyZoltanPeter]

Use of ref object of... is to achieve familiar shape for everyone also enables real inheritance features of nim.
Yes concept can be useful and interesting exercise I agree, here what we gain with the broker concept is real runtime mock-able interface out of the box.
=> https://github.com/NagyZoltanPeter/nim-brokers/blob/master/doc/OOP_Brokers.md#testing--mocking-providers

[Ivansete-status]

I saw mountMessagingClient proc got deleted. Then I checked that make test tests/api/test_api_health.nim seems to not compile.

[NagyZoltanPeter]

Yes, tests are not yet adapted. WIP

[Ivansete-status]

Suggested change

	mc: MessagingClient
	rcm: ReliableChannelManager
	messagingClient: MessagingClient
	reliableChannelManager: ReliableChannelManager

It is better to be explicit on attribute names, but this is a tiny detail. Something more relevant is that messagingClient is lazily created whereas reliableChannelManager is eagerly created. Why this difference?

I'm not saying is wrong, I just want to learn why the lazy init is needed in those cases.

[NagyZoltanPeter]

Ok, I agree.

Initialization difference is coming from the use case difference.
We all the time need kernel/waku instance regardless how we start.
MessagingClient is needed if we start as a client - startAsClient - that is eagerly init messagingClient ahead of call messaging interface accessor. While we can't say the user will need reliableChannelManager until the user explicitly ask for it.

[Ivansete-status]

This part is very interesting. Thanks :)
It inspired this: logos-messaging/nim-ffi#80 because this is a real need to allow using class-handle OOP style types instead of just flat usage.

[NagyZoltanPeter]

Happy to inspire nim-ffi... but raises the question why mimic features exists and usable right now.
Right now I don't see advance of nim-ffi over using nim-brokers.

[Ivansete-status]

I respect your opinion and I will always do.

For now, I believe it is better to focus our energies on having a proper Nim-only layering and keep using nim-ffi.

nim-ffi is the current FFI solution that we have. Let's use it for now. And if something is not good, file an issue there and we'll prioritize asap.

[Ivansete-status]

These two startAs* look very interesting too, even though they seem private and not used so far but I like the idea to differentiate by use case. ( I know they are exposed through FFI but I like more the explicit .ffi. pragma to annotate a proc that is crossing FFI boundaries.)

[NagyZoltanPeter]

Ok, there is a broker machinery behind these "overrides" - they are not real overrides because every call through either the interface reference or the implementation reference is go through broker delivery.
So when you call startAsNode from nim it translates to StartAsNode.request(...).
It has nothing to do with FFI, but enables that too.
The brokers comes with this exact concept, you don't code for FFI, you code in nim without care from where it will be used. That's how it makes using it natural and similar to use as nim library interface or using from other languages (That's what it aims if you look at the generated FFI interfaces - they look exactly how you defined them in Nim).

So to say look at the startAs... interfaces not as FFI interfaces but library interfaces.
Probably the one good exercise can be to rewrite wakunode2 with the use of the new API shape.

[Ivansete-status]

Okay,
Why perform that broker machinery request, StartAsNode.request(...), if all the components are directly accessible to the LogosDelivery object?

[NagyZoltanPeter]

That's hidden in this case.
From dev point you issue logosDelivery.startAsNode().
What's inside is part of interfacing. Particularly it the OOP styled broker interface is just a convenient look and feel - a facade.
You could simply do declare your interface as naked Request/EventBrokers - as per the original meaning so you don't need to know who is LogosDelivery class and instance. That would be a pure naked - and optimal - decoupled interface.

With the facade you can write:

let logosDelivery = LogosDeliveryInterface.create()
discard (await logosDelivery.startAsNode(...)).valueOr:
   error ...

or

let logosDelivery = LogosDeliveryInterface.create()
(await logosDelivery.startAsClient(Core, "logos.dev")).isOkOr:
   ....

let rcm = logosDelivery.channels().valueOr:
   error ... 
let channelId = (await rcm.createReliableChannel(...)).valueOr:
   error ....

let requestId = (await rmc.sendOnChannel(channelId, @[1,2,3], false)).valueOr:
   error ....

Based on the interface only.

[Ivansete-status]

Interesting!, may elaborate the purpose of these two?

[NagyZoltanPeter]

setupProviders (yes I was wrong) belongs to FFI machinery - that is a mandatory interface that is being called right from the processing thread at startup. My concept need a single point where at least initializer and shut down request brokers are instantiated - implemented. Other than that every other broker can be lazily provided

It is perfectly doable that kernel api is not available just in special case... not provided request broker is just an error return, nothing breaks.

[NagyZoltanPeter]

providerFactory is a real factory interface hook where you are able to implement conditional implementation instantiation.

[Ivansete-status]

Okay, by "conditional implementation" you mean for example the lazy init of ReliableChannelManager?

[NagyZoltanPeter]

No, it's the implementation point for dependency injection - useful when needed.
So you decide what to instantiate, based upon you config / condition.

It's being used when some says:

let instance = LogosDeliveryInterface.create()

it allows you to define arguments also of course.

[Ivansete-status]

Thanks for the recent simplification. Nevertheless, I see the PR is too complex and is adding deep architectural changes.

I envision that the next movement we need to have in master is to create a plain Nim LogosDelivery type that composes Waku, MessagingClient, ReliableChannelManager.

The LogosDelivery type suggested in this PR promotes a dynamic dispatch through method but I believe is not needed now. That's fine for a POC for the long-term, though.

[NagyZoltanPeter]

Thanks for the recent simplification. Nevertheless, I see the PR is too complex and is adding deep architectural changes.

I envision that the next movement we need to have in master is to create a plain Nim LogosDelivery type that composes Waku, MessagingClient, ReliableChannelManager.

Let me disagree a bit here. Due I feel no gain with only doing that.
The POC is coming in shape that's better call feat.
Think of it as a first step of a long way to shake the code into better shape.
Hence it's looking a bit unnecessary here and there, because I did not wanted to introduce deep architectural change now, but advocating to going toward it in small steps. This API interface step opens up such possibilities that we can utilize in further refactors.

The LogosDelivery type suggested in this PR promotes a dynamic dispatch through method but I believe is not needed now.

method here is just a distinction - or better say a suggestive/familiar look - to practice request broker
wiring (connect interface with implementation)
That's what I mean, seem not necessary now, but can be a gate opener for later refactoring that can make our code structured better.

That's fine for a POC for the long-term, though.

We are on the same page :-)

[fcecin]

@Ivansete-status @NagyZoltanPeter - TLDR: We need this PR merged to be able finish every other API / config issue that we have downstream from these improvements, so FYI I'm counting on this being merged somewhat soon. Otherwise I'll have to start working on something that will end up being a duplicate of this, essentially, so we can do the API shape refactor (overrides), the new config objects, the new kernel API, and Store API access.

[NagyZoltanPeter]

@Ivansete-status @NagyZoltanPeter - TLDR: We need this PR merged to be able finish every other API / config issue that we have downstream from these improvements, so FYI I'm counting on this being merged somewhat soon. Otherwise I'll have to start working on something that will end up being a duplicate of this, essentially, so we can do the API shape refactor (overrides), the new config objects, the new kernel API, and Store API access.

Currently all tests build and run, libraries and their examples, wakunode2 builds and can run.
Last step is wiring kernel_interface with Waku as implementation of it (notice this step will not change the shape of current use, but enables also further steps).

[github-actions]

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3946

Built from 8526a7b

[fcecin]

A layer is what you'd draw as a solid "product" box in a diagram: it's something you can remove and replace with something else. You'd be able to come up with the concept of reliable channels later and implement them on top of the messaging product. We could implement two or three channels-like features on top of the messaging layer, each with different features and implementation strategies, and people would be able to choose to mount any one of them.

Channels isn't really like that. It's more of a subsystem of the messaging layer. If you conceive of the reliable channels feature as an actually-independent product layer then it would be done slightly differently.

Now, it's fine (and good) to keep channels where it is right now in the source tree, but we can treat it as a component (that's what the directories are) that together with the messaging component, implements the Messaging/Client layer. So we have two layers only instead of three, and we avoid forcing a level of decoupling between subsystems of what is essentially the same layer.

If we go with that interpretation, then this is no longer a layering violation (by definition) and is in fact entirely fine. So what we'd do is not attribute the qualifier "layer" to a thing that just happens to sit into some height in the source tree. Just because it has a neatly separate directory doesn't mean it is a software layer.

So we would be productizing "Kernel" and "Messaging," and that's going to be the only two layers in the foreseeable future. If we want that interpretation, which also rhymes with "startAsClient" and "startAsNode" -- one start per actual product, per actual layer.

We could also switch this to emit a ChannelErrorEvent later, but that's beside this point here -- the point is that that's an intra-layer decision then.

[NagyZoltanPeter]

What I really meant, is this sounds more like a feature request for messaging client - but it might just result creating noise. Here the MessageErrorEvent is used for internal communication (which can be valid at a point) and will turn to Channel related error event.
The only problem user maybe confused he/she can subscribe to both layer messages.

[fcecin]

Yep, sure. I'm just latching onto the "layering violation" claim there before this idea spreads. This is my fault; I came up with the "three layers" idea and I have to collect that trash before it spreads further. We have two layers, not three. Channels is a subsystem, not a layer, so we may have ergonomy problems or API exposure problems (as you describe there) but not "accessing a thing from another layer." That's all I'm saying.

[fcecin]

I have added my PR review and suggested fixes as a separate PR that can be merged into this PR branch: #3966

There's no need to merge it. It's just there to demonstrate the fixes. It can be cherry-picked, reinterpreted with other code changes here, or just closed.

EDIT: @NagyZoltanPeter

[Ivansete-status]

Thanks for the recent changes!
I'm adding some more comments after a third round of review but in this occasion, deeper.
I will try to complete the review this evening or next week.
Cheers!

[Ivansete-status]

Dead variable

Suggested change

let asString = pretty(jsonNode)

[Ivansete-status]

nit, this should not be async. The use of not async code within async proc may impact the chronos loop performance.
Applies elsewhere.

Suggested change

	method getNodeInfo(
	self: LogosDelivery, id: NodeInfoId
	): Future[Result[string, string]] {.async.} =
	method getNodeInfo(
	self: LogosDelivery, id: NodeInfoId
	): Result[string, string] =

[NagyZoltanPeter]

Ok, it can be done.

[Ivansete-status]

We should not invoke quit in so low level. This should be only in app code, not in API defs. Let's make it return the error up to the caller.

[NagyZoltanPeter]

Yes, it's a leftover because meanwhile nim-brokers are fixed to manage new ctors with Result[T, string] and Future[Result[T, string]] signatures as well. So we can move back to error managing new ctor.

[Ivansete-status]

These two methods are never invoked.

Suggested change

	method subscribe(
	self: MessagingClient, contentTopic: ContentTopic
	): Future[Result[void, string]] {.async.} =
	return self.node.subscriptionManager.subscribe(contentTopic)
	method unsubscribe(
	self: MessagingClient, contentTopic: ContentTopic
	): Future[Result[void, string]] {.async.} =
	return self.node.subscriptionManager.unsubscribe(contentTopic)

[NagyZoltanPeter]

Not yet, it is to replace api procs under logos_delivery/waku/api.
My view those entry points place is at MessagingClient not under Waku.
So current is an intermediate state where both lives together.

[Ivansete-status]

This should be annotated as public so that it is easier for future lookups.
Also, I see future confusion when someone else comes and comes across this method that is marked as private but it can be used somewhere else.

Suggested change

	method send(
	method send*(

[NagyZoltanPeter]

Great catch, I need to cross check this in nim-brokers if it supported and if not, need a fix.

[Ivansete-status]

What is the purpose of createUnderContext proc? It seems it does some great unknown magic. I'd like to have something more explicit and clearer, though.

In any case, this is not an idiomatic manner of creating a ref object. It should be MessagingClient.new(...).

This also applies to ReliableChannelManager below.

[NagyZoltanPeter]

Yes, the create and creatUnderContext is BrokerImplementation machinery.
That uses and embeds the ref object's new and takes care about broker context setup for the whole implementation wiring.
So the implementation classes can be instantiated with the new ctor of course still, but create or createUnderContext is used to make fulfill the ancestor interface.

[Ivansete-status]

All these ~35 added methods are never invoked by any other Nim module.

They are trying to collapse something that is already properly separated, well defined, and controlled, within library/kernel_api/protocols/* and these new suggested methods are there to eventually replace them.

Therefore, all these methods are out of scope of the needed Nim API refactor.

[NagyZoltanPeter]

Interesting, but I think the mean of layering APIs and unify access to them is a goal. So in other words any nim module would use our library, should also access protocol layer through KernelInterface and not via deeper level, knowing protocols, peer manager, etc.
I think even the ffi layer should not live outside logos_delivery folder in the future. WDYT?

[Ivansete-status]

I think we should have one clear entry point, i.e., LogosDelivery, that composes the three layers. Then this entry point can be eventually be consumed by any other conumer: lib; REST; or another Nim app.

[NagyZoltanPeter]

Yes, than the only question is the abstract interface here, no? Becuase this POC suggest, that any can useLogosDelivery as nim library through it's interface (as factory provider will instantiate the right implementation).

[igor-sirotin]

@NagyZoltanPeter PR description formatting seem to be broken, like code snippets are "inverted".
Can you please take a look and fix it?

[NagyZoltanPeter]

@NagyZoltanPeter PR description formatting seem to be broken, like code snippets are "inverted". Can you please take a look and fix it?

Thanks, it good now.

[igor-sirotin]

Shouldn't this be in CLAUDE.md of nim-brokers itself?
Or if it must retain here, then can we put it in ./docs?

[igor-sirotin]

Can you please explain what's the benefit of using brokers here?

I'm not a Nim expert, but it seems to me that the same behaviour could be achieved with plain Nim, using concept and generics. Wouldn't it be simpler?

[igor-sirotin]

My main concern regarding this PR is how many things are presented together. It's not only about "structure api interface", but it:

• introduces use of nim-brokers at the API level
• brings gitnexus and updates agents instructions
• adds onelogosdelivery target (although as a demo)
• Change WakuMode
• Renames events
• Changes Reliable Channels API
• etc.

These are all reasonable changes. But:

1. Because e.g. there's no agreement regarding use of nim-brokers, all other changes are hold and not merged.
2. Yet it's dead weight for this specific PR. It's fog for reviewers. And my browser can barely handle it 😄

Nobody loves long-living PRs.
Extracting these changes into smaller PRs would definitely increase the overall review speed.

[igor-sirotin]

Is there an example how would the use of this API look in Nim?