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.

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

chore(rln-relay): refactor mounting procedure #1457

Merged
merged 4 commits into from
Dec 13, 2022
Merged

Conversation

rymnc
Copy link
Contributor

@rymnc rymnc commented Dec 9, 2022

Refactors the mounting procedure of waku-rln-relay

  • Updates the barrel to include the contract
  • Remove cyclic dependency on waku_node
  • Fix tests

@rymnc rymnc self-assigned this Dec 9, 2022
@rymnc rymnc added the track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications label Dec 9, 2022
@rymnc rymnc added this to the Release 0.14.0 milestone Dec 9, 2022
@rymnc
Copy link
Contributor Author

rymnc commented Dec 9, 2022

Checks are intermittently failing due to an unrelated error -

2022-12-09T10:02:24.8182700Z     /Users/runner/work/nwaku/nwaku/tests/v2/test_waku_filter.nim(172, 18): Check failed: await pushHandlerFuture.withTimeout(1.seconds)

@rymnc rymnc marked this pull request as ready for review December 9, 2022 10:15
@rymnc rymnc requested review from LNSD and s1fr0 December 9, 2022 10:15
@LNSD
Copy link
Contributor

LNSD commented Dec 9, 2022

Checks are intermittently failing due to an unrelated error -

2022-12-09T10:02:24.8182700Z     /Users/runner/work/nwaku/nwaku/tests/v2/test_waku_filter.nim(172, 18): Check failed: await pushHandlerFuture.withTimeout(1.seconds)

Could you file this failure in #1357?

@status-im-auto
Copy link
Collaborator

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
ea205ac #1 2022-12-09 23:03:11 ~15 min macos 📄log

@rymnc
Copy link
Contributor Author

rymnc commented Dec 12, 2022

Could you file this failure in #1357?

Already exists :) (the last one)

@rymnc rymnc mentioned this pull request Dec 12, 2022
11 tasks
Copy link
Contributor

@LNSD LNSD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ✅

A comment for future refactoring work:

If I understand the RLN module correctly, we have two mount protocol modes selected by the rlnRelayDynamic configuration flag.

To boost this module's extensibility and maintainability (e.g. testability), we should generalize the RLN protocol by extracting an interface that covers both modes and provides this behaviour through the constructor (check the driver interface and how we "inject" it into the Waku archive instance).

We can have a synchronous analysis and design session if needed 😁

@rymnc
Copy link
Contributor Author

rymnc commented Dec 12, 2022

A comment for future refactoring work:

Agreed, I'll do that as part of #1370, thanks for your review :)

Comment on lines +134 to +135
# set up three nodes
# node1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these repeted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it isn't, if you're referring to different tests that setup 3 nodes, they test something different each time ;)

return err("dynamic rln-relay could not be mounted: " & rlnRelayRes.error())
let wakuRlnRelay = rlnRelayRes.get()
if persistCredentials:
# TODO should be replaced with key-store with proper encryption
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this comment can be removed now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! addressed in e8f4aaa

Copy link
Contributor

@s1fr0 s1fr0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great PR!

@rymnc rymnc merged commit 33e9d8b into master Dec 13, 2022
@rymnc rymnc deleted the rln-relay-mounting branch December 13, 2022 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants