Skip to content

feat!: Use Waku API for libwaku#3490

Closed
fryorcraken wants to merge 11 commits intomasterfrom
waku-api/network-config
Closed

feat!: Use Waku API for libwaku#3490
fryorcraken wants to merge 11 commits intomasterfrom
waku-api/network-config

Conversation

@fryorcraken
Copy link
Collaborator

@fryorcraken fryorcraken commented Jul 3, 2025

Description

Demonstrate the Waku API defined in logos-messaging/specs#65 in libwaku.

Dependencies

Here are a list of changes to make this PR happen:

Changes

  • ...
  • ...

@fryorcraken fryorcraken changed the base branch from master to waku-api/autosharding-config July 3, 2025 06:41
@fryorcraken fryorcraken force-pushed the waku-api/network-config branch from c1cd55c to 23b3072 Compare July 3, 2025 06:42

]
)
nwakuCfg.RateLimits.Filter = &bindingscommon.RateLimit{Volume: 100, Period: 1, TimeUnit: bindingscommon.Second}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

doing #3489 so that I can just instantiate the rate limits here.

@github-actions
Copy link

github-actions bot commented Jul 3, 2025

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3490

Built from df62371

Base automatically changed from waku-api/autosharding-config to master July 4, 2025 07:10
@fryorcraken fryorcraken force-pushed the waku-api/network-config branch from 23b3072 to 19df60b Compare July 4, 2025 11:05
AutoSharding
StaticSharding

type AutoShadingConf* = object
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
type AutoShadingConf* = object
type AutoShardingConf* = object

@@ -0,0 +1,66 @@
import std/options, waku/factory/waku_conf, waku/factory/conf_builder
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to rename this module to libwaku_sdk_conf.nim instead, to make it clearer it is aimed for the opinionated version of the library and not for the core libwaku

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would say this would only fix the tip of the iceberg.

I think waku folder should be named lib and library can be renamed sdk

One could argue that my new config should be in api folder.

Is this addressed by the "architecture and api" virtual offsite recommendation? I havent see a forum post.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the suggestion! Let me share my 2cs in case of willing to rename folders.

I think waku folder should be named lib and library can be renamed sdk

  • waku folder is properly named IMHO. We cannot name it lib because it does not expose a library outside. Also, the current waku approach seems to be the idiomatic way of naming it. See for example: chronos chronicles
  • I would suggest keeping library folder and potentially create these folders as the need for that arises:
    • Create a new library/core/ folder and move the current library content there. libwaku-core will be compiled from there.
    • Create a new library/sdk folder that will host the opinionated libwaku-sdk. In that case, my original comment above would make much more sense.

summarizing: disregard my previous comment as it seems to be a premature optimization


Is this addressed by the "architecture and api" virtual offsite recommendation? I havent see a forum post.

We are working on it but I think this won't be covered there because that's a very specific for nwaku

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we need to rename this module to libwaku_sdk_conf.nim instead, to make it clearer it is aimed for the opinionated version of the library and not for the core libwaku

"core libwaku" should not be exposed, and we should deprecate it. the Waku API should be the sole API exposed on FFI, do we agree?

Copy link
Collaborator

Choose a reason for hiding this comment

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

"core libwaku" should not be exposed, and we should deprecate it. the Waku API should be the sole API exposed on FFI, do we agree?

I think it makes sense to only maintain Waku API, for the sake of optimizing resources. Nevertheless, core libwaku might be still relevant for future integrators. Then, IMHO we cannot strongly state that core libwaku should be deprecated. I think it will depend on its future demand.

@fryorcraken fryorcraken force-pushed the waku-api/network-config branch from b56f0f0 to 8cc1837 Compare September 9, 2025 06:50
@fryorcraken
Copy link
Collaborator Author

@waku-org/nwaku can I have feedback in terms of the location of the new artefacts?


import std/[json, atomics, strformat, options, atomics]
import chronicles, chronos, chronos/threadsync
import chronicles, chronos, chronos/threadsync, results
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need to revert

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Ivansete-status I wonder if we should rename this file to libwaku_ffi.nim as it is more focused on FFI than actual library.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think we can rename that

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

Some comments so far. Thanks! 🙌

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think we can rename that


import ./libwaku_conf

proc createNode*(config: LibWakuConf): Future[Result[Waku, string]] {.async.} =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The whole point here @Ivansete-status is to start a fresh Waku API, aligned with the spec logos-messaging/specs#65

There are 2 ways forward:

  1. Modify existing functions
  2. Create new functions

I went with (2) because until we have create, send, subscribe, it's going to be hard to dogfood. Also, makes it easier to not force upgrade to the new API in status-go integration, etc.

@@ -0,0 +1,116 @@
import
Copy link
Collaborator

Choose a reason for hiding this comment

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

If possible, I would not add this file. It seems it adds an additional layer of possible bugs, when sending attributes between layers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are defining a new API. These is the config for this new API. The existing API uses WakuNodeConf which is the CLI args one, and hence not adapted.

The intent is to move away from WakuNodeConf and have a config object that works for application developers.

@fryorcraken
Copy link
Collaborator Author

Superseded by #3564 due to nim-lang/nimble#1466

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.

2 participants