chore: moving node initialization code to node_factory.nim #2479

gabrielmer · 2024-02-27T10:27:39Z

Description

As a first step towards refactoring app.nim, we're moving code related to node initialization to a new factory/middleware layer.

A new directory named waku/factory was created, which will contain all logic related to this functionality.
In addition, introduced a file named node_factory.nim, which will contain the core logic of this layer.
The idea is to be able to pass down node configurations and get a node in response.

A PR will follow extending the functionalities in node_factory beyond moving existing code.

Changes

creating factory directory and factory/node_factory.nim file
moving node initialization procs from app.nim to factory/node_factory.nim
moving internal_config.nim, external_config.nim andwakunode2_validator_signed.nim from apps/wakunode2 to waku/factory
moving builder.nim from waku/node to waku/factory directory
renaming wakunode2_validator_signed.nim to validator_signed.nim

Issue

#2441

gabrielmer · 2024-02-27T10:31:13Z

waku/node/node_factory.nim

+  ../../apps/wakunode2/external_config,
+  ../../apps/wakunode2/internal_config,
+  ../../apps/wakunode2/wakunode2_validator_signed


I have mixed feelings regarding importing code from the apps directory.
On one hand, the node's configuration parameters are defined there and we need them in order to create a node.

On the other, code related to the app should be separate from the the waku node modules.

Not sure if we should move the configs out of the app directory or what would be the most appropriate way to go

Good point! What if we move the external_config, internal_config, and wakunode2_validator_signed modules into the folder waky/node/?

Sounds good :))

So took a look and was able to move internal_config.nim and wakunode2_validator_signed but moving external_config.nim caused some conflicts because of its code related to parsing CLI commands, which also makes sense that is part of the apps directory.

Ideally we should decouple a node's configuration options to the application's CLI parameters.
The decoupling would make sense only if we find a way to do it without duplicating everything. So I left external_config.nim in apps until we find a better solution.

github-actions · 2024-02-27T10:32:19Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2479

Built from 38862f8

github-actions · 2024-02-27T11:28:07Z

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.

SionoiS

LGTM

I would avoid naming things node. I suggest factory instead of node_factory and maybe naming the folder initialization (or init).

gabrielmer · 2024-02-27T14:07:46Z

I would avoid naming things node. I suggest factory instead of node_factory and maybe naming the folder initialization (or init).

Mmm makes sense, having the word node is kind of redundant. On the other hand, it does make clear that it isn't code specific to a protocol and that it's related to the WakuNode object itself. I think it makes things easier to understand for me, but don't have a strong opinion.

@Ivansete-status @NagyZoltanPeter thoughts?

alrevuelta · 2024-02-27T14:23:13Z

waku/node/validator_signed.nim

@@ -19,9 +19,9 @@ import
 const MessageWindowInSec = 5*60 # +- 5 minutes

 import


unsure if @jm-clius agrees.

Better having the signed validator in apps? the idea is to have all the pieces to build a Waku node with standard features under waku/node (but without adding it to the WakuNode object itself).

Thought that a signed validator entered into the set of "standard features"

However, these elements (app-level validation, app's DB config, etc.) were moved out of the node for a reason. This (incorrectly) brings back awareness of the app environment into the node object, while the app environment should know about the node and not the other way around. Amongst other things, you're now seeing a lot of interesting circular imports (such as internal_config depending on waku_node, while waku_node also depends on internal_config).

Perhaps the solution is rather to raise the bindings to the app level? Of course, creating a convenient app-level node factory or middleware that can be wrapped in bindings will be part of this effort.

the solution is rather to raise the bindings to the app level and, of course, create a convenient app-level node factory or middleware.

👍

Makes sense! Maybe all of this should be at app level.

Notice that here we did not change the node object at all - we only moved the procs that initialize nodes from the app file to a node_factory file inside waku/node directory which isn't used by any other file related to the node.

Or in other words, we're attaching to waku_node a separate "kit" to easily create a node. So once you import waku_node you get both the basic procs to do everything from scratch and a separate already built "node factory" with standard implementations.

Regarding the circular imports, notice that those two are different files. internal_config depends on waku/node/waku_node.nim which has the actual WakuNode implementation. And then there's waku/waku_node.nim that imports files from the waku/node directory and exports them as a bundle so apps don't have to import all the files individually.
Here we're adding the node factory to the second one, to waku/waku_node.nim. But we are not touching the node's implementation.

Maybe we should have all of this at the app level. But the only thing that would change here is the file path, the logic is already separate.

Right, good point about the import here - waku/waku_node.nim is just a barrel import. However, it remains a bit weird that there are now "app" imports in modules that live very closely with the node. It indeed only affects import and directory structure afaics, but I guess what we need is:
a) node (the wrapper around libp2p switch)
b) app middleware (config, factory, validators, etc. that's used by all apps, including ones using the bindings)
c) specific apps, e.g. wakunode2 - bare minimum to use the app middleware to create a specific type of waku application, such as wakunode2 binary.

This layered approach should also reflect in the import structure (outer app can import app middleware can import node, but never the other way).

Sounds great! Really like that idea of a new "middleware" layer - agree 100% that it's a much more logically sound structure.

Will re-organize the it like that.
Thanks so much! 🤩

NagyZoltanPeter · 2024-02-28T06:24:11Z

I would avoid naming things node. I suggest factory instead of node_factory and maybe naming the folder initialization (or init).

Mmm makes sense, having the word node is kind of redundant. On the other hand, it does make clear that it isn't code specific to a protocol and that it's related to the WakuNode object itself. I think it makes things easier to understand for me, but don't have a strong opinion.

@Ivansete-status @NagyZoltanPeter thoughts?

Looking at the content of the source file, it's not a true factory for me, in terms of factory pattern.
I would rather call it an initializer or configurator.
Also it implies the knowledge how to use, when to call these procs, rather embed all this dependencies. Idk if this opinion helps...

Ivansete-status · 2024-02-28T08:45:47Z

I would avoid naming things node. I suggest factory instead of node_factory and maybe naming the folder initialization (or init).

Mmm makes sense, having the word node is kind of redundant. On the other hand, it does make clear that it isn't code specific to a protocol and that it's related to the WakuNode object itself. I think it makes things easier to understand for me, but don't have a strong opinion.

@Ivansete-status @NagyZoltanPeter thoughts?

I personally prefer being explicit with node_factory , but I agree it sounds redundant to have node/node_factory, so either option is good to me :)

gabrielmer · 2024-02-28T08:55:54Z

Looking at the content of the source file, it's not a true factory for me, in terms of factory pattern. I would rather call it an initializer or configurator. Also it implies the knowledge how to use, when to call these procs, rather embed all this dependencies. Idk if this opinion helps...

Yes! So the idea is that it will be an actual factory. We're first trying to move the existing code and dependencies there so when we add its main code the diff won't be enormous filled with copy-pasted things. It would be difficult to differentiate what's new code and what's copy-pasted procs being moved around.

So this PR is only so the following PRs are more scoped and easier to review

gabrielmer · 2024-02-28T18:30:47Z

Went over everything today with @Ivansete-status today

The new "middleware" layer is now in the directory waku/factory/.
We settled for that naming as it describes more precisely its current purpose for creating/initiating nodes. If at any point it becomes a more generalized layer it can be renamed.

All the imports should now follow the hierarchical logical structure.

Notice that this is only the first step of a bigger refactor task. This PR is intended as a reordering of existing logic and no new code is introduced. The next step is to build on top on this and add functionalities to node_factory.nim to make it an actual factory.

NagyZoltanPeter

I think its a great step ahead with in mind it will evolve. Thank you!

jm-clius

Thanks!

jm-clius · 2024-02-29T12:12:50Z

apps/chat2/chat2.nim

@@ -33,6 +33,7 @@ import
  ../../waku/waku_node,
  ../../waku/node/waku_metrics,
  ../../waku/node/peer_manager,
+  ../../waku/factory/builder,
  ../../waku/common/utils/nat,
  ./config_chat2


Eventually it may be possible for all apps, including chat2, to start using the common factory/external_config and only add app-specific config items in an extension module. Nothing to action for now. :)

Ivansete-status

Great movement! Thanks for it! 💯
I didn't check the detail of every block as I assume is block movement.
Cheers

…d from app to node modules

gabrielmer commented Feb 27, 2024

View reviewed changes

gabrielmer requested review from NagyZoltanPeter and Ivansete-status February 27, 2024 10:56

gabrielmer self-assigned this Feb 27, 2024

gabrielmer requested a review from SionoiS February 27, 2024 10:56

gabrielmer marked this pull request as ready for review February 27, 2024 10:58

gabrielmer marked this pull request as draft February 27, 2024 11:34

SionoiS approved these changes Feb 27, 2024

View reviewed changes

alrevuelta reviewed Feb 27, 2024

View reviewed changes

gabrielmer force-pushed the chore-moving-code-to-node-factory branch from 49f0062 to d845d1f Compare February 28, 2024 17:15

gabrielmer marked this pull request as ready for review February 28, 2024 18:21

gabrielmer requested a review from jm-clius February 28, 2024 18:30

NagyZoltanPeter approved these changes Feb 28, 2024

View reviewed changes

gabrielmer force-pushed the chore-moving-code-to-node-factory branch from 99d5c38 to f9badca Compare February 29, 2024 10:00

jm-clius approved these changes Feb 29, 2024

View reviewed changes

Ivansete-status approved these changes Feb 29, 2024

View reviewed changes

gabrielmer added 7 commits March 2, 2024 16:37

Moving node initialization code to node_factory.nim

67834b4

removing setupProtocols from app

4ff9eb7

removing startNode from app

765ff3d

removing comments

30f76c6

moving external_config, internal_config and wakunode2_validator_signe…

cd3150a

…d from app to node modules

renaming wakunode2_validator_signed to validator_signed

4857657

fixing import

f7faf36

gabrielmer added 9 commits March 2, 2024 16:40

moving external_config.nim back to apps directory

49c1bd7

reordering into factory module

b19da32

moving validator_signed to factory directory

7ec013b

Fixing test and cleaning diff

e54ceca

more diff cleaning

f01e18c

adding missing import

dac239f

missing import

9d20eec

moving builder.nim to factory

88b6554

remove incorrect import

2d3710e

gabrielmer force-pushed the chore-moving-code-to-node-factory branch from 82788f3 to 2d3710e Compare March 2, 2024 22:42

gabrielmer merged commit 361fe2c into master Mar 3, 2024
9 of 10 checks passed

gabrielmer deleted the chore-moving-code-to-node-factory branch March 3, 2024 00:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: moving node initialization code to node_factory.nim #2479

chore: moving node initialization code to node_factory.nim #2479

gabrielmer commented Feb 27, 2024 •

edited

Loading

gabrielmer Feb 27, 2024

Ivansete-status Feb 27, 2024

gabrielmer Feb 27, 2024

gabrielmer Feb 27, 2024

github-actions bot commented Feb 27, 2024 •

edited

Loading

github-actions bot commented Feb 27, 2024

SionoiS left a comment

gabrielmer commented Feb 27, 2024

alrevuelta Feb 27, 2024

gabrielmer Feb 27, 2024

jm-clius Feb 27, 2024 •

edited

Loading

SionoiS Feb 27, 2024 •

edited

Loading

gabrielmer Feb 27, 2024 •

edited

Loading

jm-clius Feb 27, 2024

gabrielmer Feb 27, 2024

NagyZoltanPeter commented Feb 28, 2024

Ivansete-status commented Feb 28, 2024

gabrielmer commented Feb 28, 2024

gabrielmer commented Feb 28, 2024

NagyZoltanPeter left a comment

jm-clius left a comment

jm-clius Feb 29, 2024

Ivansete-status left a comment

		@@ -19,9 +19,9 @@ import
		const MessageWindowInSec = 5*60 # +- 5 minutes

		import

chore: moving node initialization code to node_factory.nim #2479

chore: moving node initialization code to node_factory.nim #2479

Conversation

gabrielmer commented Feb 27, 2024 • edited Loading

Description

Changes

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 27, 2024 • edited Loading

github-actions bot commented Feb 27, 2024

SionoiS left a comment

Choose a reason for hiding this comment

gabrielmer commented Feb 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jm-clius Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

SionoiS Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

gabrielmer Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NagyZoltanPeter commented Feb 28, 2024

Ivansete-status commented Feb 28, 2024

gabrielmer commented Feb 28, 2024

gabrielmer commented Feb 28, 2024

NagyZoltanPeter left a comment

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

gabrielmer commented Feb 27, 2024 •

edited

Loading

github-actions bot commented Feb 27, 2024 •

edited

Loading

jm-clius Feb 27, 2024 •

edited

Loading

SionoiS Feb 27, 2024 •

edited

Loading

gabrielmer Feb 27, 2024 •

edited

Loading