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

Implement simple lottery. #97

Merged
merged 2 commits into from
Dec 13, 2024
Merged

Implement simple lottery. #97

merged 2 commits into from
Dec 13, 2024

Conversation

tolikzinovyev
Copy link
Member

@tolikzinovyev tolikzinovyev commented Dec 10, 2024

Content

Implement simple lottery's prove and verify functions. This PR doesn't have tests. Let's see what property tests will test and if it still makes sense to add other tests.

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)

Issue(s)

Closes #69

Copy link
Collaborator

@rrtoledo rrtoledo 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 would just add (a couple of) function(s) for the node to send a proof and the aggregator to check an individual element either exposing in algo or creating in the wrapper.
Doing so, we would support decentralized settings,

  • the node can check locally if it has to send an element
  • the aggregator could verify a single element as soon as they arrive, push them on a local and vector and cast it as a Proof when succesfully verifying the last one needed

We either can have 2 functions, separating the node from the aggregator, or one use by both.

src/simple_lottery/algorithm.rs Outdated Show resolved Hide resolved
src/simple_lottery/algorithm.rs Outdated Show resolved Hide resolved
Copy link
Member Author

@tolikzinovyev tolikzinovyev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only planned to create an interface for the centralized setting. Let's work on the decentralized interface separately? I would be a different wrapper according to design doc. @rrtoledo

src/simple_lottery/algorithm.rs Outdated Show resolved Hide resolved
@rrtoledo
Copy link
Collaborator

I only planned to create an interface for the centralized setting. Let's work on the decentralized interface separately? I would be a different wrapper according to design doc. @rrtoledo

Sure let's do it in a separate PR.
As for the wrapper, we should reuse the existing code so we can see whether we should have 2 wrappers for centralised and decentralised settings in wrapper.rs or not.

Copy link
Collaborator

@curiecrypt curiecrypt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Are you planning to add/enhance public function docs in another PR?
  • I would write a simple test function even though the protests will cover most of the cases.
  • Also, I saw a typo in file simple_lottery/init.rs line 27: bound_completess. FYI :)
    LGTM 👍

src/simple_lottery/algorithm.rs Outdated Show resolved Hide resolved
@tolikzinovyev
Copy link
Member Author

@curiecrypt

Are you planning to add/enhance public function docs in another PR?

There is an open issue to do this for all files. #65 Not sure if I will have time this week though.

I would write a simple test function even though the protests will cover most of the cases.

I'm afraid that if I write a test, it would become obsolete when property tests are implemented. However, if they do not test something, I would be happy to revisit this!

Also, I saw a typo in file simple_lottery/init.rs line 27: bound_completess. FYI :)

Thanks, will make a PR with a fix.

Copy link
Collaborator

@curiecrypt curiecrypt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@tolikzinovyev
Copy link
Member Author

@rrtoledo waiting for your LGTM as promised :)

Copy link
Collaborator

@rrtoledo rrtoledo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with changes agreed to do in #65

@rrtoledo rrtoledo merged commit 480e277 into main Dec 13, 2024
1 check passed
@rrtoledo rrtoledo deleted the tolik/simple-lottery-algo branch December 13, 2024 09:40
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.

Simple Lottery Construction - Prove/Verify
4 participants