-
Notifications
You must be signed in to change notification settings - Fork 54
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
Prerequistes for RLN integration tutorial #1031
Conversation
Jenkins BuildsClick to see older builds (2)
|
Thank you @kgrgpg for the PR, it looks good in general, a few comments before getting it merged:
|
Co-authored-by: Sanaz Taheri Boshrooyeh <[email protected]>
Co-authored-by: Sanaz Taheri Boshrooyeh <[email protected]>
@staheri14 All the recommendations are now incorporated in the branch. |
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 resolving the comments! though, in general, it is conventional to let the reviewer mark his/her comments as resolved and not the other way around :)
The PR looks good to me, some nitpick:
- please add a period
.
to the end of all the lines for consistency - Also, please remove your Infura node addresses from the pictures (keep the
wss
part but cover the rest).
Your branch is out of date with the master branch, please also update it.
|
||
![](https://i.imgur.com/UFsoRHR.jpg) | ||
|
||
9. Close the settings and now you should see the test networks in the drop down on the top right |
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.
please change the picture so that the Goerli network is check-marked instead of the ethereum mainnet.
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.
Please add a period .
to the end of all the lines (including here) for consistency.
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.
LGTM. I followed the tutorial 👍 and it worked.
Imo, it would be nice to have short section at beginning listing the main steps:
- setup metamask
- setup infura node
This allows operators/users to know right from the start what to expect.
Also, it would be helpful to provide a link to a tutorial explaining how setup the interaction between Waku node and infura.
Further, it might be helpful to provide a short intro to what the purpose of the infura node is,
and why operators should setup an infura account.
Maybe mention something like: Infura provides a simple straight-forward way of setting up endpoints for interaction with the Ethereum chain and the Waku RLN smart contract without having to run a dedicated Ethereum node. Setting up infura is not mandatory. Operators concerned with the centralized aspect introduced by infura can setup their own node.
Bump. New recommendations incorporated. |
|
||
![](https://i.imgur.com/PfGmq4i.jpg) | ||
|
||
ß4. Select Gorli network in Endpoints. |
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.
The picture is not rendered properly.
ß4. Select Gorli network in Endpoints. | |
4. Select Gorli network in Endpoints. |
|
||
3. After creating the project, you will be presented with a dashboard like follows. Note that your Project Id and secret will be different. | ||
|
||
![](https://i.imgur.com/PfGmq4i.jpg) |
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.
Your main-net node addresses is still visible in this pic :)
I checked your PR, and it looks good, I added a few more editorial comments, once you addressed those, feel free to squash and merge your PR. |
@kaiserd thanks for your comment! this part will be covered in the extended version of the current tutorial for which I am going to create a PR. |
@kaiserd Thanks for this suggestion as well, noted for the follow-up PR. |
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.
LGTM
One idea would be to in-line the images instead of relying on external provider (imgur). These are likely to disappear at some point.
Co-authored-by: Sanaz Taheri Boshrooyeh <[email protected]>
Co-authored-by: Sanaz Taheri Boshrooyeh <[email protected]>
Co-authored-by: Sanaz Taheri Boshrooyeh <[email protected]>
These are the tutorials on the pre-requisites needed to integrate the RLN contract with nWaku.