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: Proving for the playground #50

Merged
merged 1 commit into from
Jan 27, 2023

Conversation

Dominik1999
Copy link
Contributor

Closes #19

This PR enables proving to the playground.

There are some open questions regarding the UX and the proving step code itself:

  • UX

    • At the moment, for the user running and proving a program looks equal. The output is almost the same.
    • We should add some info that proving might take a while (see Game of Life example), maybe even a pop-up that prints something during the proof
    • There is no way to show the proof atm
  • Proving step

    • We don't expose in Miden v0.3 any info about the trace length or cycles or even time to prove in ProgramOutputs or ExecutionTrace, so we can't get the numbers (maybe I am mistaken, but I didn't find it)
    • I couldn't find a way to pass the proof from Rust to Typescript (frontend) via wasm-bindgen. There seems to be a way to return complex structures from Rust to Typescript (frontend) but I didn't understand exactly how

@grjte
Copy link
Contributor

grjte commented Jan 26, 2023

Since this will need to be rebased and updated after the repo structure PR is changed, let's keep it as a draft until then. I'm going to put it into draft mode and remove my review request. Once it's been rebased, please mark it as "ready for review" and then re-request review

@grjte grjte marked this pull request as draft January 26, 2023 09:58
@grjte
Copy link
Contributor

grjte commented Jan 26, 2023

There are some open questions regarding the UX and the proving step code itself:

* UX

* * At the moment, for the user running and proving a program looks equal. The output is almost the same.

I think this is addressed by discussions below. As long as we have a way to provide/show the proof then it's fine for the output to be the same (imo). Is there something else you were thinking about here? Also, I think deeper considerations about UX should probably wait until we are actually focusing on design. At the moment, I think we should focus on functionality & a modular structure so we can update the design easily once we have someone working on it

* * We should add some info that proving might take a while (see Game of Life example), maybe even a pop-up that prints something during the proof

We can use react-hot-toast to do this very easily: https://react-hot-toast.com/

* * There is no way to show the proof atm

I think we should create a new issue for this, but reference #19 inside that issue, as well as the other related open questions/issues you've noted

* Proving step

* * We don't expose in Miden v0.3 any info about the trace length or cycles or even time to prove in `ProgramOutputs` or `ExecutionTrace`, so we can't get the numbers (maybe I am mistaken, but I didn't find it)

I think this is part of a larger conversation in the VM, and related to changes for debugging improvements as well (0xPolygonMiden/miden-vm#580). We should bring it up on the team sync call, but maybe this should be separated out from this set of playground chanages and become its own working group that looks at the 2 things together.

* * I couldn't find a way to pass the proof from Rust to Typescript (frontend) via wasm-bindgen. There seems to be a way to return complex structures from Rust to Typescript (frontend) but I didn't understand exactly how

This seems to be related to the comment above about having a way to show the proof, so it seems to me that we should just create a new issue for showing the proof.

@Dominik1999 Dominik1999 force-pushed the dominik-add-proving-to-playground branch from 5765eb5 to 8ffcac2 Compare January 26, 2023 12:12
@Dominik1999 Dominik1999 marked this pull request as ready for review January 26, 2023 12:18
@Dominik1999
Copy link
Contributor Author

Since this will need to be rebased and updated after the repo structure PR is changed, let's keep it as a draft until then. I'm going to put it into draft mode and remove my review request. Once it's been rebased, please mark it as "ready for review" and then re-request review

Thanks, I just rebased.

@Dominik1999
Copy link
Contributor Author

* * I couldn't find a way to pass the proof from Rust to Typescript (frontend) via wasm-bindgen. There seems to be a way to return complex structures from Rust to Typescript (frontend) but I didn't understand exactly how

This seems to be related to the comment above about having a way to show the proof, so it seems to me that we should just create a new issue for showing the proof.

Here is a new issue about getting and showing the proof. We can tackle this in the next milestone. #52

@grjte
Copy link
Contributor

grjte commented Jan 27, 2023

Here is a new issue about getting and showing the proof. We can tackle this in the next milestone. #52

Sounds good! I think we should also create an issue about adding toast messages with https://react-hot-toast.com/ and put that in the "nice to have" for this milestone. wdyt?

Copy link
Contributor

@grjte grjte left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! I left a few very minor comments. Also, I want to be sure all inline TODOs are captured in issues. I think many/most are, but just to check

playground/miden-wasm/src/lib.rs Show resolved Hide resolved
playground/miden-wasm/src/lib.rs Outdated Show resolved Hide resolved
playground/miden-wasm/src/lib.rs Show resolved Hide resolved
playground/miden-wasm/src/lib.rs Show resolved Hide resolved
@Dominik1999 Dominik1999 force-pushed the dominik-add-proving-to-playground branch from 8ffcac2 to 83bed3d Compare January 27, 2023 10:04
Copy link
Contributor

@grjte grjte left a comment

Choose a reason for hiding this comment

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

Approved! Thank you!

Comment on lines +81 to +82
// TODO: We must catch all cases of inputs. Input can be not a valid json,
// input can be empty, stack_init can be empty, advice_tape can be empty. And all combinations.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I see - you're talking about cases of input errors. I would probably have said "all cases of input errors", but the message is now clear, and no need to update, especially since it'll be tackled in the next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah indeed. I like it those nits

@Dominik1999 Dominik1999 force-pushed the dominik-add-proving-to-playground branch from 83bed3d to c24c692 Compare January 27, 2023 10:20
@Dominik1999 Dominik1999 merged commit f946f62 into main Jan 27, 2023
@Dominik1999 Dominik1999 deleted the dominik-add-proving-to-playground branch January 27, 2023 10:25
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.

Add ability to prove a program as well
2 participants