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.

Sign up for GitHub

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

Jump to bottom

fix: setting up node with modified config #3036

Merged
merged 2 commits into from
Sep 16, 2024
+16 −15
Merged

fix: setting up node with modified config #3036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba7acf2
setting up node with modified config
gabrielmer Sep 12, 2024
23d6a77
fix tests
gabrielmer Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions apps/wakunode2/wakunode2.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,15 @@ when isMainModule:
nodeHealthMonitor = WakuNodeHealthMonitor()
nodeHealthMonitor.setOverallHealth(HealthStatus.INITIALIZING)

var confCopy = conf

let restServer = rest_server_builder.startRestServerEsentials(
nodeHealthMonitor, conf
nodeHealthMonitor, confCopy
).valueOr:
error "Starting esential REST server failed.", error = $error
quit(QuitFailure)

var waku = Waku.init(conf).valueOr:
var waku = Waku.init(confCopy).valueOr:
error "Waku initialization failed", error = error
quit(QuitFailure)

Expand All @@ -77,12 +79,12 @@ when isMainModule:
quit(QuitFailure)

rest_server_builder.startRestServerProtocolSupport(
restServer, waku.node, waku.wakuDiscv5, conf
restServer, waku.node, waku.wakuDiscv5, confCopy
).isOkOr:
error "Starting protocols support REST server failed.", error = $error
quit(QuitFailure)

waku.metricsServer = waku_metrics.startMetricsServerAndLogging(conf).valueOr:
waku.metricsServer = waku_metrics.startMetricsServerAndLogging(confCopy).valueOr:
error "Starting monitoring and external interfaces failed", error = error
quit(QuitFailure)

Expand Down
4 changes: 2 additions & 2 deletions tests/wakunode2/test_app.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ include waku/factory/waku
suite "Wakunode2 - Waku":
test "compilation version should be reported":
## Given
let conf = defaultTestWakuNodeConf()
var conf = defaultTestWakuNodeConf()

let waku = Waku.init(conf).valueOr:
raiseAssert error
Expand All @@ -43,7 +43,7 @@ suite "Wakunode2 - Waku initialization":

test "node setup is successful with default configuration":
## Given
let conf = defaultTestWakuNodeConf()
var conf = defaultTestWakuNodeConf()

## When
var waku = Waku.init(conf).valueOr:
Expand Down
17 changes: 8 additions & 9 deletions waku/factory/waku.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,16 +101,15 @@ proc validateShards(conf: WakuNodeConf): Result[void, string] =

## Initialisation

proc init*(T: type Waku, conf: WakuNodeConf): Result[Waku, string] =
var confCopy = conf
proc init*(T: type Waku, confCopy: var WakuNodeConf): Result[Waku, string] =
Comment on lines -105 to +104
Copy link
Collaborator

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?

Copy link
Contributor Author

@gabrielmer gabrielmer Sep 16, 2024 •
edited
Loading

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:

  1. 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

  2. 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)

  3. In tests/wakunode2/test_app.nim, had to change from let to var for the configurations passed to init() now that we modify the object in the proc

Let me know if there's one of these points you think we could spare

Copy link
Contributor

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...

Copy link
Contributor Author

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

let rng = crypto.newRng()

logging.setupLog(conf.logLevel, conf.logFormat)
logging.setupLog(confCopy.logLevel, confCopy.logFormat)

# TODO: remove after pubsubtopic config gets removed
var shards = newSeq[uint16]()
if conf.pubsubTopics.len > 0:
let shardsRes = topicsToRelayShards(conf.pubsubTopics)
if confCopy.pubsubTopics.len > 0:
let shardsRes = topicsToRelayShards(confCopy.pubsubTopics)
if shardsRes.isErr():
error "failed to parse pubsub topic, please format according to static shard specification",
error = shardsRes.error
Expand All @@ -120,9 +119,9 @@ proc init*(T: type Waku, conf: WakuNodeConf): Result[Waku, string] =

if shardsOpt.isSome():
let relayShards = shardsOpt.get()
if relayShards.clusterId != conf.clusterId:
if relayShards.clusterId != confCopy.clusterId:
error "clusterId of the pubsub topic should match the node's cluster. e.g. --pubsub-topic=/waku/2/rs/22/1 and --cluster-id=22",
nodeCluster = conf.clusterId, pubsubCluster = relayShards.clusterId
nodeCluster = confCopy.clusterId, pubsubCluster = relayShards.clusterId
return err(
"clusterId of the pubsub topic should match the node's cluster. e.g. --pubsub-topic=/waku/2/rs/22/1 and --cluster-id=22"
)
Expand Down Expand Up @@ -191,8 +190,8 @@ proc init*(T: type Waku, conf: WakuNodeConf): Result[Waku, string] =
let node = nodeRes.get()

var deliveryMonitor: DeliveryMonitor
if conf.reliabilityEnabled:
if conf.storenode == "":
if confCopy.reliabilityEnabled:
if confCopy.storenode == "":
return err("A storenode should be set when reliability mode is on")

let deliveryMonitorRes = DeliveryMonitor.new(
Expand Down
Loading