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
feat: Add incentivization PoC for RLNaaS in Lightpush #3166
base: master
Are you sure you want to change the base?
feat: Add incentivization PoC for RLNaaS in Lightpush #3166
Changes from 22 commits
a3bef81
22852d6
83953a5
4e956c4
5ddf3af
5167eb6
829c1f6
718c772
d7f2a33
363d56c
ef4c2d3
03eb690
a92ffc7
4b60383
586dadd
e517eca
3b5ea31
3356d40
4ad3bda
4c57cbc
e9df168
af16879
c7d4e68
2bb2d0f
d9a86c8
3d026e7
133a411
60aaed6
ccec7f8
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.
It is generally better to avoid external dependencies in tests. I suggest taking a look on how
Anvil
is being used in other RLN tests.For example,
tests/waku_rln_relay/test_rln_group_manager_onchain.nim
andwaku/waku_rln_relay/constants.nim
can serve as a good reference :) ( noticeEthClient* = "http://127.0.0.1:8540"
is defined in constants and it deals with a localAnvil
instance )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.
Creating a new instance of
web3
for every call toisEligibleTxId
doesn't sound very efficient. I believe it is better to do that once, when the app starts, and only close theweb3
instance at the app's end.I'd suggest creating a new type
EligibilityManager
that contains theweb3
instance.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 understand the idea high-level and it makes total sense. I started implementing it in ccec7f8 (for now, it replicates the current behavior, just with the
EligibilityManager
instance on top).I'm not sure about the next steps though. How to make the manager start "when the app starts" and close "at the app's end"? Where are the app's start and end defined? Do we have similar examples in our codebase I should look into? I'd note also that this PR doesn't intend to integrate the funcitonality into the app itself; all eligibility-related functions are only called from tests. Is this detail relevant?
I'd appreciate any pointers!
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.
Why we can ensure that comment?
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.
Strictly speaking, we cannot.
The number
21000
comes from the Ethereum spec (yellow paper) as the gas cost that every transaction pays. A simple transfer pays just that, and a contract call pays extra costs on top.Note: the Yellow paper is outdated, as is admitted in its repo:
TBH, I don't know how to make sense of that last link. But I would assume that as long as the 21000 constant has remained unchanged throughout Ethereum's history (AFAIK), it's unlikely to be changed.
Another question is that if the payments in question happen on a rollup, technically, the rollup developers can tweak gas costs for their EVM implementation. Linea, for one, also uses 21000 here.
In summary, the assumption "a tx is a simple transfer if and only if it uses 21000 gas" is somewhat of a hack, but haven't come up with a better way to check whether the tx is a contract call or not.