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

feat(Rln relay): Improves rln-chat2 interface, displays links to the registration tx, warns about spam messages #1082

Merged
merged 3 commits into from
Aug 24, 2022

Conversation

staheri14
Copy link
Contributor

@staheri14 staheri14 commented Aug 19, 2022

Thie PR is a progress towards #1069
Addresses a subset pf comments re "chat interface", namely:

  • Warns users when about their spamming activity
  • Provides users with a link to their registration tx on Goerli testnet

@staheri14 staheri14 self-assigned this Aug 19, 2022
@staheri14 staheri14 added the track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications label Aug 19, 2022
@staheri14 staheri14 requested review from jm-clius and LNSD August 19, 2022 00:32
@LNSD
Copy link
Contributor

LNSD commented Aug 19, 2022

I understand that at this moment the goal is to have a prototype where the user is using the command-line. So the comments below are just suggestions 😁

IMO, writing to the stdout (via echo, or via any other means) should only happen from the main application, the nodewaku2 in our case.

Can we make the "warning" an event that can be consumed in the future by other APIs (e.g., an android/ios app that wraps the nwaku implementation). In that case, you cannot use the stdout to notify the user.

So in this case I would create a callback to the "warning event" and hook it in nodewaku2 and call echo inside the callback method.

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to mark as ready for review when it is. Agree with @LNSD that the chat2 toy application can do with a cleanup, though this is an effort separate from RLN.

@staheri14 staheri14 marked this pull request as ready for review August 19, 2022 23:17
@staheri14
Copy link
Contributor Author

staheri14 commented Aug 19, 2022

IMO, writing to the stdout (via echo, or via any other means) should only happen from the main application, the nodewaku2 in our case.

Can we make the "warning" an event that can be consumed in the future by other APIs (e.g., an android/ios app that wraps the nwaku implementation). In that case, you cannot use the stdout to notify the user.

So in this case I would create a callback to the "warning event" and hook it in nodewaku2 and call echo inside the callback method.

@LNSD I believe that the current implementation is also event-driven. The mountRlnRelay proc accepts a callback function to be triggered when the registration tx gets mined. The callback function is defined and passed by the chat2 user (the main program) and contains the print statement. Do you suggest something different than this?

@staheri14 staheri14 requested a review from jm-clius August 19, 2022 23:22
@staheri14
Copy link
Contributor Author

@jm-clius Thanks, forgot to mark it as ready when added the reviewers. It is now marked as ready!

@staheri14
Copy link
Contributor Author

staheri14 commented Aug 19, 2022

@LNSD I see, I think your suggestion is about the publish proc which prints a warning about spamming. I can also update the interface of that proc to accept a callback function. Already on it.

Update: This implies a bit of careful refactoring (as publish proc is nested inside a few other procs),I don't want to rush the implementation, so opened a tracking issue to address it in a follow up PR #1083 Feel free to continue the conversation on this matter on the tracking issue.

@status-im-auto
Copy link
Collaborator

status-im-auto commented Aug 20, 2022

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ eae530e #1 2022-08-20 04:06:44 ~19 min macos 📦bin
eae530e #2 2022-08-21 22:13:50 ~29 min windows 📄log
eae530e #3 2022-08-22 22:46:55 ~3 min windows 📄log

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 ✅

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

LGTM!

@staheri14 staheri14 force-pushed the rln-relay/chat2-intreface-improvement branch from edef3f2 to 6e9740c Compare August 24, 2022 20:39
@staheri14 staheri14 mentioned this pull request Aug 24, 2022
15 tasks
@staheri14 staheri14 merged commit bc29619 into master Aug 24, 2022
@staheri14 staheri14 deleted the rln-relay/chat2-intreface-improvement branch August 24, 2022 22:47
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