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

refactor!: refactored transcript traits and impls #648

Merged
merged 3 commits into from
Aug 8, 2024

Conversation

alxiong
Copy link
Contributor

@alxiong alxiong commented Aug 6, 2024

As observed in https://www.notion.so/espressosys/Fiat-Shamir-Transcript-f29ccc7a23c94178aac5cfa5d336fd63 and discussed on Zulip, we decide to remove get_and_append_challenge() and only use get_challenge() and also introduce back state for solidity transcript as most other projects do.

This partially undone some changes made in #619.

This PR:

  • Updated trait Transcript
    • change get_and_append_challenge() to get_challenge()
    • add get_n_challenges() for multiple challenges for the same rounds
    • rename append_challenge() to append_field()
    • add append_fields()
  • Updated struct SolidityTranscript: adding back state and squeeze logic (see code comment)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (main)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added relevant changelog entries to the CHANGELOG.md of touched crates.
  • Re-reviewed Files changed in the GitHub PR explorer

@alxiong alxiong requested a review from mrain as a code owner August 6, 2024 15:56
@alxiong alxiong changed the base branch from main to commonprefix-patch August 6, 2024 15:57
Copy link
Contributor

@philippecamacho philippecamacho left a comment

Choose a reason for hiding this comment

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

Left some minor suggestions.
LGTM.

@alxiong
Copy link
Contributor Author

alxiong commented Aug 8, 2024

All of your comments are addressed in 2956aa5

I'll update the CHANGELOG when we merge all changes into main eventually.

@alxiong alxiong merged commit 5c61e3e into commonprefix-patch Aug 8, 2024
5 checks passed
@alxiong alxiong deleted the refactor-transcript branch August 8, 2024 03:05
alxiong added a commit that referenced this pull request Sep 4, 2024
* feat: append G2 point from SRS to transcript (#647)

* append G2 point from SRS to transcript

* use powers_of_h[1] instead of beta_h

* refactor!: refactored transcript traits and impls (#648)

* refactor transcript trait and impl

* align transcript with 32byte word size

* address comments

* update changelog and jf-plonk version
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