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: allowing configuration of application level callbacks #3206

Merged
merged 12 commits into from
Dec 13, 2024

Conversation

gabrielmer
Copy link
Contributor

Description

We noticed that libwaku doesn't automatically configure a callback for receiving Relay messages when creating a node. Currently, the user has to manually subscribe to each topic providing their callback.

In order to make libwaku more user friendly, we need to be able to register all the event-emitting callbacks automatically when creating a node. Therefore, I added a WakuCallbacks type where a user can pass all the different callbacks they need when creating a node. Currently, only relayHandler is defined in the type, but in a following PRs new callbacks will be added (for example, a callback that runs every time a topic health changes)

Changes

  • introduced WakuCallbacks type for application-level callbacks
  • added setupCallbacks procedure that adds the configured WakuCallbacks
  • modified Waku.new() to accept WakuCallbacks and properly set them up
  • introduced a callbacks parameter to libwaku's createWakuso we can configure in node creation all the event-emitting callbacks

Issue

#3076

Copy link

github-actions bot commented Dec 11, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3206

Built from 8b2241a

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.

Thanks so much for it! I like the concept very much but I think we need to address a couple of minor details :)
Nice PR! 🥳

waku/factory/waku.nim Outdated Show resolved Hide resolved
waku/factory/waku.nim Outdated Show resolved Hide resolved
waku/factory/waku_callbacks.nim Outdated Show resolved Hide resolved
Copy link
Member

@richard-ramos richard-ramos left a comment

Choose a reason for hiding this comment

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

Would set_event_callback stil lbe necessary with this new AppCallbacks feature?

@gabrielmer
Copy link
Contributor Author

gabrielmer commented Dec 13, 2024

Would set_event_callback stil lbe necessary with this new AppCallbacks feature?

My understanding is that it is still necessary because there's two type of callbacks - the callbacks at the nwaku's application level which in libwaku we use to emit events (the type of callback of this PR), and a callback defined at libwaku's client to handle all these events emitted by the application level callbacks.

Or in other words the callbacks of this PR are run whenever something in nwaku happens, for example when a Relay message is received.
The nwaku user can execute whatever (Nim) code they want whenever that happens. And in libwaku we take advantage of it by emitting events on those callbacks, which will be then handled by the callback set in libwaku's set_event_callback

Let me know if I might be missing something or if you think of a way to simplify this structure :)

@gabrielmer gabrielmer merged commit 049fbea into master Dec 13, 2024
9 of 11 checks passed
@gabrielmer gabrielmer deleted the chore-configure-external-callbacks branch December 13, 2024 16:38
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.

3 participants