Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Refinements of RLN on mainnet spec #34
Refinements of RLN on mainnet spec #34
Changes from 6 commits
f15b30a
586d70b
818ce98
82b4bc7
8d7a41b
2a1b433
dfe5e53
5ef1d30
258a07e
0f9d680
fd91933
3a1aabb
ca0c416
eb4dfd6
cc1a917
090c65c
7c29616
66945c7
b8ea666
80ba2bd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do we agree @richard-ramos that for now, we don't include this feature in the smart contract?
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 way it works right now is that you can:
Since for any option ideally the client should call
estimateGas
to determine which option is cheaper, and the smart contract's been already modified to support this scenario, we can keep it. Unless we want to remove the tracking of the membership age ordering in the smart contract completely? (i.e. we will not know in the smart contract what's the oldest membership to expire). If so, then, cool. It means less gas for the user, and nwaku can track the age of the memberships then, and choose which memberships to expire.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.
Are you saying that nwaku would need to listen to emit events from the smart contract? I think we definitely do not want that as the whole point of onchain tree was to not have to load all events.
However, can the solution be:
Meaning that it's cheaper to insert or expire (no ordering at insertion). But it's possible to get all data from the contract API (no tracing of emitted events).
The ordering can then be done by the applicaiton (nwaku, jswaku) etc
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.
Sure! no need to use events. There's already a function that would let you iterate over all the memberships registered. Also perhaps in the future we might want to integrate with the graph and obtain this information from there too.
I'll propose the changes on a separate branch from waku-org/waku-rlnv2-contract#13 so it's possible to compare both options. I do think not having to track the ordering in the contract will be very beneficial due to reduced costs