-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update on the rln registration figure to match the current spec #497
Conversation
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.
Looks good to me! Left two minor comments
|
||
..., --- [ label = " " ]; | ||
b=>a [ label = "Insert(pk, index)"]; | ||
b=>d [ label = "Insert(pk, index)"]; |
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 contract should return (pk, index) to nodes to allow them to update their local trees (I assumed so, since you removed the getAuthPath/getRoot calls).
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.
IIRC contract won't return this, since transaction hasn't been mined yet, but you have to listen for it to be emitted as an event.
See e.g. https://github.com/ChihChengLiang/poseidon-tornado/blob/main/contracts/Tornado.sol#L66 and https://docs.ethers.io/v5/concepts/events/
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.
IIRC contract won't return this, since transaction hasn't been mined yet, but you have to listen for it to be emitted as an event.
The figure is a simplified version of actual interaction with the contract, and is clearly not precise. I have revised it to better reflect the mining phase as well. Done in 002a812
static/rfcs/17/rln-relay.msc
Outdated
|||; | ||
d abox d [ label=" \n Listening to the membership contract \n "] ; | ||
|
||
a box a [ label=" \n Generate sk,pk \n "] ; | ||
a=>b [ label = " \n Register(pk, x ETH) \n " ] ; |
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.
Can the label go on top of the arrow rather over it?
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.
Yes, it can, done in f0f4d58
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.
I also notice Github has nice image diffs now!
|
||
..., --- [ label = " " ]; | ||
b=>a [ label = "Insert(pk, index)"]; | ||
b=>d [ label = "Insert(pk, index)"]; |
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.
IIRC contract won't return this, since transaction hasn't been mined yet, but you have to listen for it to be emitted as an event.
See e.g. https://github.com/ChihChengLiang/poseidon-tornado/blob/main/contracts/Tornado.sol#L66 and https://docs.ethers.io/v5/concepts/events/
Yes, and it is a very useful feature! |
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.
Great!
* master: RFC16: add version call (#505) fix(noise): update RFC to implementation (#508) fixup: 37/WAKU2-NOISE fix images paths (#506) New RFC: 37/WAKU2-NOISE-SESSIONS (#504) 36/WAKU2-BINDINGS-API (#501) docs(16/WAKU2-RPC): add ENR to waku info (#502) Adding 35/WAKU2-NOISE to menu (#500) add RFC33 to index (#499) feat: 32/RLN raw spec New RFC: 35/WAKU2-NOISE (#496) Update on the rln registration figure to match the current spec (#497) 33/WAKU-DISCV5: Add first raw version (#487) Add pubsubTopic field to index (#492) Fix markdown links (#493) Categorize 22 & 31 (#490) Changed PB Timestamp index to 10 (#491) 13/14/16/21: Change in timestamp format (#483) add: RFC31 copyright statement (#489) 17/WAKU-RLN-RELAY: Revise spec for its draft version (#484)
* master: RFC16: add version call (#505) fix(noise): update RFC to implementation (#508) fixup: 37/WAKU2-NOISE fix images paths (#506) New RFC: 37/WAKU2-NOISE-SESSIONS (#504) 36/WAKU2-BINDINGS-API (#501) docs(16/WAKU2-RPC): add ENR to waku info (#502) Adding 35/WAKU2-NOISE to menu (#500) add RFC33 to index (#499) feat: 32/RLN raw spec New RFC: 35/WAKU2-NOISE (#496) Update on the rln registration figure to match the current spec (#497) 33/WAKU-DISCV5: Add first raw version (#487) Add pubsubTopic field to index (#492) Fix markdown links (#493) Categorize 22 & 31 (#490) Changed PB Timestamp index to 10 (#491) 13/14/16/21: Change in timestamp format (#483) add: RFC31 copyright statement (#489) 17/WAKU-RLN-RELAY: Revise spec for its draft version (#484)
The rln registration figure is outdated and does not match the specs specifically the contract behavior is not correct. This PR contains the necessary amendments.
I have also taken a copy of the previous figure in a separate file in case we may want to switch to/support the previous design.