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

Add Clone support for ProofWithPublicInputsTarget #826

Closed
wants to merge 1 commit into from
Closed

Add Clone support for ProofWithPublicInputsTarget #826

wants to merge 1 commit into from

Conversation

tikhono
Copy link

@tikhono tikhono commented Nov 25, 2022

When I have done some recursive example, I found out the lack for Clone derive for ProofWithPublicInputsTarget.

Here example: https://gist.github.com/tikhono/6b3222ccaa2030280efcbc7038eeb373

Does the usage of clone correct? Or lack of this derive is intendant, so there are some reasonings for this?

@dlubarov
Copy link
Contributor

Thanks for sending, this sees fine to me if it's needed. That said I think there are a couple other potential fixes,

  • It looks like our current recursion examples call set_proof_with_pis_target before verify_proof. It does seem a bit odd to require that ordering though.
  • We could change verify_proof to make proof_with_pis a reference. I think we usually take references to "target" structs (besides small ones which impl Copy); AFAIK there wasn't a particular reason we took a value in this case.

Do you think the reference change would address the issue for you?

@tikhono
Copy link
Author

tikhono commented Nov 25, 2022

Yes, I think reference should work for me.

Thanks for fast response!

@dlubarov
Copy link
Contributor

Sounds good! I made #828 to change that, it might just be a bit delayed as our build is broken at the moment (rust-lang/rust#105037)

@tikhono tikhono closed this Nov 30, 2022
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