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: Add Hash impls for EmbeddedCurvePoint and EmbeddedCurveScalar #6970

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Jan 7, 2025

Description

Problem*

Replaces #6918

Summary*

Thanks to @skaunov for the original issue. This needed to be broken into a separate PR since I couldn't suggest these changes on the original.

Implements Hash for two types so that they can be put in HashMaps, etc.

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@jfecher jfecher requested a review from a team January 7, 2025 14:39
@jfecher jfecher enabled auto-merge January 7, 2025 14:40
Copy link
Contributor

github-actions bot commented Jan 7, 2025

Compilation Memory Report

Program Peak Memory %
keccak256 78.54M 0%
workspace 123.63M 0%
regression_4709 422.96M 0%
ram_blowup_regression 1.58G 0%
rollup-base-public 202.76M 100%
rollup-base-private 202.76M 100%
private-kernel-tail 168.30M -17%
private-kernel-reset 168.33M -77%
private-kernel-inner 168.30M -43%

Copy link
Contributor

github-actions bot commented Jan 7, 2025

Execution Memory Report

Program Peak Memory %
keccak256 74.68M 0%
workspace 123.88M 0%
regression_4709 315.98M 0%
ram_blowup_regression 512.50M 0%
rollup-base-public 202.76M -74%
rollup-base-private 202.76M -53%
private-kernel-tail 168.30M -8%
private-kernel-reset 168.33M -35%
private-kernel-inner 168.30M -22%

@jfecher jfecher added this pull request to the merge queue Jan 7, 2025
@TomAFrench TomAFrench removed this pull request from the merge queue due to a manual request Jan 7, 2025
@TomAFrench
Copy link
Member

Seeing a lot of red here.

@jfecher
Copy link
Contributor Author

jfecher commented Jan 7, 2025

@TomAFrench looks like those external repos have their own trait with a different Hash function. Now that these types implement std::hash::Hash, it is being resolved to that instead leading to the parameter count error. Those external libraries in aztec-packages will need to be updated.

We may need #6895 to be merged first and to update aztec-packages to only import the trait containing the no argument hash function.

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.

4 participants