-
Notifications
You must be signed in to change notification settings - Fork 57
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
refactor: big refactor to add waku component in libwaku instead of only waku node #2658
refactor: big refactor to add waku component in libwaku instead of only waku node #2658
Conversation
You can find the image built from this PR at
Built from 92f68d9 |
You can find the image built from this PR at
Built from 92f68d9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small comment, there are tools/applications which may use the App* export, can we rename them too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thank you!
What about the make file? Needs to be changed too no? |
If you refer to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, as I understand the general direction taken here. However, have asked a question on switching to ptr
operations below.
proc getRunningNetConfig(app: App): AppResult[NetConfig] = | ||
var conf = app.conf | ||
let (tcpPort, websocketPort) = getPorts(app.node.switch.peerInfo.listenAddrs).valueOr: | ||
proc getRunningNetConfig(waku: ptr Waku): Result[NetConfig, string] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard for me to see why in this module we have to switch to operations on ptr
s. It causes some uncomfortable integrations into higher layer applications: startWaku(addr waku)
is quite a bit less intuitive than waku.startWaku()
. My assumption is that this is necessary for the thread management in the bindings? However, wouldn't it be better to wrap this into another ptr
operation layer specifically (and only) for the bindings? May be missing some fundamental limitation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment @jm-clius ! 🙌
I agree that it becomes more awkward to use with startWaku(addr waku)
Let me elaborate more on why this is needed:
-
We should make the
startWaku
proc async because thewaitFor
statement should be only invoked from the highest level to avoid deadlocks. i.e. thewaitFor
should only be invoked from within thewakunode2
code or highest level oflibwaku
.It is not correct to have a
waitFor
as we had before this PR. See:
Lines 177 to 179 in 853ec18
proc startApp*(app: var App): AppResult[void] = let nodeRes = catch: (waitFor startNode(app.node, app.conf, app.dynamicBootstrapNodes)) -
The
startWaku
(formerly known asstartApp
) changes the state of the given parameter,waku
, but we cannot longer pass avar
parameter because thestartWaku
should beasync
(commented in the previous point) and it doesn't compile (<<cannot be captured as it would violate memory safety>>
.) -
Given that we cannot use
var
, the only way to allow changing the state of a variable in anasync
proc, is by usingptr
type, which is considered unsafe but in our case we can make sure that the passedWaku
instance has longer lifetime than the call ofstartApp
and it won't crash because of incorrect address being used inside thestartApp
.
Nevertheless, there might be a more elegant approach ( cc @arnetheduck :)
Description
With this PR we:
app.nim
module towaku.nim
App
type toWaku
type. We start consideringWaku
as the type that containsWakuNode
, discovery elements, and will contain an instance ofPeerManager
in the future.Waku
type represents Waku ;P (very original name ;P)libwaku
code so that we start dealing with theWaku
object from withinlibwaku
and therefore, start having a similar behaviour as thewakunode2
appIssue
migrate DiscV5 and DNS Discovery from app.nim to waku_node.nim
- #2452