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: group data #1

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat: group data #1

wants to merge 7 commits into from

Conversation

agazso
Copy link
Contributor

@agazso agazso commented Dec 7, 2023

This PR adds 2 new smart contracts:

  • CypherCity has the application level functionality with functions such as joinGroup and addToGroup. It also has some helper functions (getMerkleRootCreationDate, getMerkleTreeRoot) that are either required for offchain proof verification or helps with optimizations. It interacts with the CypherCitySemaphore smart contract.
  • CypherCitySemaphore: this is a thin wrapper over the Semaphore smart contract, exposing internal functionality.

Some notes:

  • The joinGroup functionality is meant to be private, but I kept it public for now because it makes testing easier. Probably there is a pattern how to achieve this better.
  • getMerkleRootCreationDate needs to be exposed so that the proofs of older messages can be verified offchain.
  • getMerkleTreeRoot needs to be exposed so that it is cheap to check if the membership has changed.
  • I spent a lot of time to make sensible tests, because they are actually integration tests and the order of the tests may change what is the environment they are executed in (they are non-deterministic). The simplest was to deploy a new set of contracts before each test runs and therefore make the environment more or less deterministic. The tradeoff is that this makes tests run much longer and with more tests added we may run out of the time allowed by the CI. Maybe there is a better way to do that. Also it would be good to omit the hardhat output.
  • There is one test that is failing, but from the output it seems like it executes the test properly, which then results in a reverted transaction. However the error message (Error: cannot estimate gas) suggests that there is a problem with gas, but I think this is an environment or configuration issue.

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.

2 participants