Skip to content
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

Merged
merged 4 commits into from
Mar 26, 2024

Conversation

s-tikhomirov
Copy link
Contributor

@s-tikhomirov s-tikhomirov commented Feb 12, 2024

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:

  • new data types: EligibilityProof (which, for now, is thought to be a transaction id) and EligibilityStatus;
  • data types DummyRequest (with EligibilityProof) and DummyResponse (with EligibilityStatus) for testing purposes (to be replaced by the request and response structures of the corresponding protocol, e.g. Store); (left for future work)
  • encode and decode procedures that convert Nim data types into protocol buffers and back, respectively;
  • tests to ensure that encoding and decoding work correctly: decode(encode(X)) == X.

How to test

./env.sh bash
nim c -r ./tests/incentivization/test_rpc_codec.nim

@s-tikhomirov s-tikhomirov changed the title Incentivization: add codec for eligibility proofs feat: Incentivization: add codec for eligibility proofs Feb 12, 2024
Copy link

github-actions bot commented Feb 12, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2419-rln-v2-false

Built from 6693d21

Copy link
Contributor

@jm-clius jm-clius left a 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.

waku/incentivization/rpc.nim Outdated Show resolved Hide resolved
waku/incentivization/rpc.nim Outdated Show resolved Hide resolved
waku/incentivization/rpc.nim Outdated Show resolved Hide resolved
waku/incentivization/rpc.nim Outdated Show resolved Hide resolved
waku/incentivization/rpc.nim Outdated Show resolved Hide resolved
waku/incentivization/rpc_codec.nim Outdated Show resolved Hide resolved
waku/incentivization/rpc_codec.nim Outdated Show resolved Hide resolved
waku/incentivization/rpc_codec.nim Outdated Show resolved Hide resolved
Copy link
Collaborator

@Ivansete-status Ivansete-status left a 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

tests/incentivization/test_rpc_codec.nim Outdated Show resolved Hide resolved
@jm-clius
Copy link
Contributor

jm-clius commented Mar 8, 2024

@s-tikhomirov anything blocking for last changes and to get this merged?

@Ivansete-status
Copy link
Collaborator

Morning @s-tikhomirov!
Shall this PR be merged or is anything blocking it?

@s-tikhomirov
Copy link
Contributor Author

Morning @s-tikhomirov! Shall this PR be merged or is anything blocking it?

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?

@jm-clius
Copy link
Contributor

@s-tikhomirov I would suggest converting to a draft PR until you're happy that the changes are ready to be reviewed and merged. cc @Ivansete-status

@s-tikhomirov s-tikhomirov marked this pull request as draft March 19, 2024 12:08
@s-tikhomirov s-tikhomirov force-pushed the feat-add-incentivization-codec branch from dfad7ea to 08c0187 Compare March 19, 2024 12:29
@s-tikhomirov s-tikhomirov marked this pull request as ready for review March 22, 2024 16:03
Copy link
Collaborator

@Ivansete-status Ivansete-status left a 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

waku/incentivization/rpc_codec.nim Outdated Show resolved Hide resolved
waku/incentivization/rpc_codec.nim Outdated Show resolved Hide resolved
tests/incentivization/test_rpc_codec.nim Show resolved Hide resolved
Copy link
Contributor

@SionoiS SionoiS left a 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.

waku/incentivization/rpc_codec.nim Outdated Show resolved Hide resolved
tests/incentivization/test_rpc_codec.nim Outdated Show resolved Hide resolved
Copy link
Contributor

@jm-clius jm-clius left a 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)

waku/incentivization/rpc.nim Outdated Show resolved Hide resolved
waku/incentivization/rpc.nim Outdated Show resolved Hide resolved
waku/incentivization/rpc_codec.nim Show resolved Hide resolved
@s-tikhomirov s-tikhomirov force-pushed the feat-add-incentivization-codec branch from 23801ff to a9c7a81 Compare March 26, 2024 16:28
@s-tikhomirov s-tikhomirov merged commit 6553026 into master Mar 26, 2024
14 of 15 checks passed
@s-tikhomirov s-tikhomirov deleted the feat-add-incentivization-codec branch March 26, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants