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: Make IOPTranscript publicly accessible #331

Merged
merged 8 commits into from
Jul 13, 2023

Conversation

tessico
Copy link
Contributor

@tessico tessico commented Jun 29, 2023

Description

The jellyfish IOPTranscript has very ergonomic wrappers around merlin, why not expose it publicly.
Also I added an extra convenience method for getting challenge bytes instead of field elements.
Not sure whether this is a good approach - instead one could make the transcript field of the struct public and allow users to call more methods directly from merlin.


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 a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

Copy link
Contributor

@chancharles92 chancharles92 left a comment

Choose a reason for hiding this comment

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

Thanks @tessico , LGTM, could you also fix the lint failure?

@tessico tessico changed the title Make IOPTranscript public feat: Make IOPTranscript publicly accessible Jun 30, 2023
@tessico
Copy link
Contributor Author

tessico commented Jun 30, 2023

Hmm I'm new to this action, but I can't find why CI is failing - the title seems to conform the guidelines, unless I'm missing sth?

@chancharles92
Copy link
Contributor

Hmm I'm new to this action, but I can't find why CI is failing - the title seems to conform the guidelines, unless I'm missing sth?

@mrain @alxiong Do you know what's the reason for PR title failing?

@mrain
Copy link
Contributor

mrain commented Jun 30, 2023

Hmm I'm new to this action, but I can't find why CI is failing - the title seems to conform the guidelines, unless I'm missing sth?

@mrain @alxiong Do you know what's the reason for PR title failing?

The reason maybe the permission setting of this PR: https://github.com/EspressoSystems/jellyfish/blob/1b03cada0ac48607fd8490a6115abc3070811757/.github/workflows/commit-title.yaml#L10C5-L10C5

Since it only checks the title, could we remove the "write" requirement here? cc @alxiong @Ancient123

@alxiong
Copy link
Contributor

alxiong commented Jul 3, 2023

the best action is go to "Setting" of this repo and grant write permission to this Action. which will be done by @Ancient123 (as I have communicated with him on Zulip)

@Ancient123
Copy link
Member

Try rebasing? If that doesn't work, I can do an administrator merge.

@alxiong
Copy link
Contributor

alxiong commented Jul 11, 2023

if we can't figure out why the commitlint CI fails, let's just merge it first, there's no problem with the commit message of this PR @mrain

@mrain
Copy link
Contributor

mrain commented Jul 11, 2023

the best action is go to "Setting" of this repo and grant write permission to this Action. which will be done by @Ancient123 (as I have communicated with him on Zulip)

I don't have access the settings of this repo. Could you try this and see if it proceeds? cc @alxiong @Ancient123
If no let's go ahead and merge it ignoring CI failure.

@alxiong alxiong merged commit 6210b1f into EspressoSystems:main Jul 13, 2023
@tessico tessico deleted the make-iop-transcipt-pub branch July 13, 2023 20:42
@tessico
Copy link
Contributor Author

tessico commented Jul 13, 2023

Thanks for this!

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