-
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
fix: setting up node with modified config #3036
fix: setting up node with modified config #3036
Conversation
You can find the image built from this PR at
Built from f081254 |
17f18b9
to
23d6a77
Compare
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.
Hm, nice catch! Thank you for it!
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 it! Just added a question
var confCopy = conf | ||
proc init*(T: type Waku, confCopy: var WakuNodeConf): Result[Waku, 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.
I actually think these are the only needed changes.
Did you try to see what happens with just that?
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.
Yes, so the need of each change is the following:
-
In this file, apart from this line, changed the name to refer that we are using a copy of the original configuration and that we're not modifying the user's input
-
In
apps/wakunode2/wakunode2.nim
, we create a copy of the original configuration and pass it to all the procs (so we don't modify the original configuration passed by the user) -
In
tests/wakunode2/test_app.nim
, had to change fromlet
tovar
for the configurations passed toinit()
now that we modify the object in the proc
Let me know if there's one of these points you think we could spare
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.
Do we use/re-use the original user configuration afterward? Just curious...
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.
Do we use/re-use the original user configuration afterward? Just curious...
After that point we actually don't use the original configuration, but did it just to not override the original user configuration which generally isn't a good pattern. In principle we can avoid doing the copy and change the original but idk, it doesn't feel right haha
Description
We currently modify a copy of our configurations when initializing a node in
nwaku/waku/factory/waku.nim
Lines 104 to 106 in 1713f56
However, we don't use this modified configuration when setting up the protocols and initializing the node, which causes mismatches between them.
Added a fix to use the same version of the configuration at the
wakunode2
application layer.Changes
wakunode2