feat(rln-relay): mounting proc waku_node, barrel imports #1379
Conversation
rymnc
added
track:production
track:rln
Jenkins BuildsClick to see older builds (2)
|
waku/v2/node/waku_node.nim
Outdated
| proc mountRlnRelay*(node: WakuNode, rlnConf: WakuRlnConfig) {.async.} = | ||
| info "mounting rln relay" | ||
|
|
||
| let rlnRelayRes = await WakuRlnRelay.init(node.wakuRelay, |
There was a problem hiding this comment.
Can the init proc be renamed to new? It is creating a ref object.
From Status Nim Styleguide:
newreturns a reference to the given type
| import | ||
| ./waku_rln_relay/waku_rln_relay_types |
There was a problem hiding this comment.
I'd instead also import the entry point module here. The current module structure is very likely to change and this line will have to change also if we don't put here the entry module.
| ./waku_rln_relay/waku_rln_relay_constants, | ||
| ./waku_rln_relay/waku_rln_relay_types, | ||
| ./waku_rln_relay/waku_rln_relay_utils, | ||
| ./waku_rln_relay/waku_rln_relay_metrics |
There was a problem hiding this comment.
Now modules can be renamed using shorter names.
| ./waku_rln_relay/waku_rln_relay_constants, | |
| ./waku_rln_relay/waku_rln_relay_types, | |
| ./waku_rln_relay/waku_rln_relay_utils, | |
| ./waku_rln_relay/waku_rln_relay_metrics | |
| ./waku_rln_relay/constants, | |
| ./waku_rln_relay/types, | |
| ./waku_rln_relay/utils, | |
| ./waku_rln_relay/protocol_metrics |
It can be done in another PR.
| ): Future[RlnRelayResult[void]] {.async.} = | ||
| # Returns RlnRelayResult[void], which indicates the success of the call | ||
| ): Future[RlnRelayResult[WakuRlnRelay]] {.async.} = | ||
| # Returns RlnRelayResult[WakuRlnRelay] |
There was a problem hiding this comment.
I feel this comment is redundant 😅
|
Mixing the "protocol mounting" with the "barrel imports" seems too much for a single PR. Is it possible to have a single piece of work per PR? One for "barrel imports" and another for the "protocol mounting"? This will ease the review process, making it more agile. In any case, the direction of these changes looks promising 😁 Good job! |
I agree, that was the idea too. However, importing the "entrypoint" created a cyclic dependency, which would then again require us to import the specific module required so that we don't touch too much code in the PR. |
It is about atomic commits/changes that, at some point, can be reverted if needed without removing other "unrelated" changes. And, as we are following the "squash and merge" strategy, this means one commit per pull request. This will help with the reviewing process making it more agile. So if possible, I would prefer two different commits/PRs. Thanks. |
|
Closing to split the PR into
|
This PR cleans up the imports for rln-relay, and also makes the mounting procedure in line with other protocols.
waku_rln_relaycreated so that only one import is requiredwaku_rln_relay_typesspecifically is required bywaku_message, therefore only that module is imported. This should be cleaned up in the future.Not included in the scope of this PR, but will be included in future PRs -