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

Remove the legacy contract and integrate the batching one #386

Merged
merged 1 commit into from
Feb 13, 2023

Conversation

iamrecursion
Copy link
Contributor

@iamrecursion iamrecursion commented Feb 10, 2023

I am so sorry for the size of this PR, but most of that horrendous line count is the new compiled contracts required for testing. It's big regardless, though.

This PR removes any use of the legacy contract from the codebase. This means that, while the code still exists due to some dependencies, it is no longer used. The batching contract is no-longer gated behind a feature flag.

It is now fully integrated with the batching contract on chain, as well as the prover service used to generate proofs for verification on chain. These interactions are tested, and while multiple components are mocked the signup sequencer portion of the code is as it should be with the real external dependencies.

Note that running the tests on macOS may trigger firewall protections that you need to disable. This depends on your macOS version.

Closes #259

@iamrecursion iamrecursion added the enhancement New feature or request label Feb 10, 2023
@iamrecursion iamrecursion self-assigned this Feb 10, 2023
@iamrecursion iamrecursion changed the base branch from main to batching/main February 10, 2023 22:55
@iamrecursion iamrecursion force-pushed the wip/ara/legacy-removal branch 2 times, most recently from e7fb2e0 to c2a7cdd Compare February 10, 2023 23:00
@codecov-commenter
Copy link

Codecov Report

Merging #386 (c2a7cdd) into batching/main (43f049a) will increase coverage by 0.75%.
The diff coverage is 80.91%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Impacted file tree graph

@@                Coverage Diff                @@
##           batching/main     #386      +/-   ##
=================================================
+ Coverage          42.36%   43.12%   +0.75%     
=================================================
  Files                 30       29       -1     
  Lines               6798     7217     +419     
=================================================
+ Hits                2880     3112     +232     
- Misses              3918     4105     +187     
Flag Coverage Δ
test 43.12% <80.91%> (+0.75%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/contracts/abi.rs 0.00% <0.00%> (ø)
src/server.rs 21.40% <0.00%> (-0.27%) ⬇️
tests/abi/mod.rs 11.90% <0.00%> (ø)
src/contracts/mod.rs 52.27% <57.89%> (+41.16%) ⬆️
src/app.rs 74.24% <66.66%> (-0.76%) ⬇️
tests/prover_mock/mod.rs 82.69% <82.69%> (ø)
tests/integration_tests.rs 91.66% <90.76%> (ø)
src/identity_tree.rs 78.64% <92.59%> (-10.72%) ⬇️
src/identity_committer.rs 79.45% <94.04%> (+18.25%) ⬆️
src/database/mod.rs 78.57% <100.00%> (+0.46%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43f049a...c2a7cdd. Read the comment docs.

@iamrecursion iamrecursion force-pushed the wip/ara/legacy-removal branch from c2a7cdd to fa8dc6c Compare February 10, 2023 23:37
@iamrecursion iamrecursion marked this pull request as ready for review February 13, 2023 15:27
@iamrecursion iamrecursion requested a review from a team as a code owner February 13, 2023 15:27
Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

Looks good in general! Few nitpicks inline.

src/app.rs Outdated Show resolved Hide resolved
src/app.rs Show resolved Hide resolved
src/contracts/mod.rs Outdated Show resolved Hide resolved
src/database/mod.rs Show resolved Hide resolved
src/identity_committer.rs Show resolved Hide resolved
This commit removes any use of the legacy contract from the codebase.
This means that, while the code still exists due to some dependencies,
it is no longer used. The batching contract is no-longer gated behind a
feature flag.

It is now fully integrated with the batching contract on chain, as well
as the prover service used to generate proofs for verification on chain.
These interactions are tested, and while multiple components are mocked
the signup sequencer portion of the code is as it should be with the
real external dependencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batch insertion
3 participants