-
Notifications
You must be signed in to change notification settings - Fork 57
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: Incentivization: add codec for eligibility proofs #2419
Conversation
You can find the image built from this PR at
Built from 6693d21 |
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 as a start. I've added some comments below, but these can be addressed in a follow-up PR. Mostly I would decouple the coding/decoding of EligibilityProof
and EligibilityStatus
from the underlying req-resp protocol. This would also mean that you probably don't need a DummyRequest
and DummyResponse
for now - you can simply build logic and tests on the raw bytes of the eligibility messages.
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 just added a minor comment
@s-tikhomirov anything blocking for last changes and to get this merged? |
Morning @s-tikhomirov! |
Morning @Ivansete-status ! Sorry for keeping this open, I've been busy with another line of work until now (academic papers). I've been thinking of addressing existing comments in the same PR, would it be OK? |
@s-tikhomirov I would suggest converting to a |
dfad7ea
to
08c0187
Compare
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, thanks!
I just added simple comments
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
Style wise I prefer to cluster logically related lines together and add spaces in between.
It's only my personal preference though.
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 think this is a better approach rather than introducing the dummy protocol. I would like to see my comments addressed, but let's do so in a subsequent PR. This one is good to merge. Comments mostly relate to mandatory vs optional fields (in protobuf all fields should be optional
for consistent, flexible wire encoding, but we can and should enforce rules around mandatory fields at the point where we decode)
23801ff
to
a9c7a81
Compare
Description
Encoding and decoding data structures that store proofs of eligibility is the first step towards a PoC implementation of incentivization for Store (and, in the future, other protocols), as outlined in #1961 and RFC 73/WAKU2-INCENTIVIZATION.
Changes
The PR implements the following:
EligibilityProof
(which, for now, is thought to be a transaction id) andEligibilityStatus
;data types(left for future work)DummyRequest
(withEligibilityProof
) andDummyResponse
(withEligibilityStatus
) for testing purposes (to be replaced by the request and response structures of the corresponding protocol, e.g. Store);encode
anddecode
procedures that convert Nim data types into protocol buffers and back, respectively;decode(encode(X)) == X
.How to test