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

Crypto: Remove repeated code in Merkle tree backends #584

Merged
merged 7 commits into from
Sep 29, 2023

Conversation

schouhy
Copy link
Contributor

@schouhy schouhy commented Sep 29, 2023

Remove repeated code in Merkle tree backends

Description

This PR leverages const generics to remove repeated code in crypto/src/merkle_tree/backends. We previously had two separate implementations for 256 and 512 bit in both batched and single element backends. This PR merges both into a single implementation for batch and a single implementation for single elements.

Type of change

  • Refactor

@schouhy schouhy requested review from ajgara and a team as code owners September 29, 2023 10:39
@codecov-commenter
Copy link

Codecov Report

Merging #584 (ce292fa) into main (c718c5d) will increase coverage by 0.00%.
The diff coverage is 98.97%.

@@           Coverage Diff           @@
##             main     #584   +/-   ##
=======================================
  Coverage   95.49%   95.49%           
=======================================
  Files         112      110    -2     
  Lines       19008    18966   -42     
=======================================
- Hits        18151    18111   -40     
+ Misses        857      855    -2     
Files Coverage Δ
crypto/src/merkle_tree/backends/batch.rs 99.03% <100.00%> (ø)
crypto/src/merkle_tree/merkle.rs 100.00% <100.00%> (ø)
crypto/src/merkle_tree/backends/single.rs 97.82% <97.82%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@MauroToscano MauroToscano left a comment

Choose a reason for hiding this comment

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

Maybe we can find a better name for singlebackend ?

@schouhy
Copy link
Contributor Author

schouhy commented Sep 29, 2023

its indeed an ugly name. Other options I considered are:

  1. FieldElementBackend instead of SingleBackend and FieldElementVectorBackend instead of BatchBackend, referring to the data they work with.
  2. Just Backend instead of SingleBackend
  3. Just Backend for both of them and use them as batch::Backend and single::Backend

@MauroToscano
Copy link
Collaborator

I think 1 Is nice. Also FEVector is more descriptive than Batch. We can change that change later though, I don't mind

@MauroToscano MauroToscano added this pull request to the merge queue Sep 29, 2023
Merged via the queue into main with commit 314fafc Sep 29, 2023
6 checks passed
@MauroToscano MauroToscano deleted the remove-repeated-code-merkle-backends branch September 29, 2023 15:47
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.

5 participants