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

Support PartialMerkleTree to be used as secret input in the .input file. #1072

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Sep 13, 2023

This small PR makes it possible to use PartialMerkleTree in the .input files as a secret input.

@Fumuran Fumuran force-pushed the andrew-support-pmt-via-cli-inputs branch 2 times, most recently from cc85497 to 2ed971f Compare September 13, 2023 02:08
@Fumuran Fumuran marked this pull request as ready for review September 13, 2023 02:23
@Fumuran Fumuran linked an issue Sep 13, 2023 that may be closed by this pull request
Copy link
Contributor

@bobbinth bobbinth 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! Not a full review yet, but I did leave a couple of nits inline.

One other comment: I think it would be good to put together an example to illustrate how different input sections can be used. This example would go here and it the input file could be something similar to what you've put together for tests in this PR.

docs/src/intro/usage.md Outdated Show resolved Hide resolved
miden/src/cli/data.rs Outdated Show resolved Hide resolved
miden/src/cli/data.rs Show resolved Hide resolved
miden/src/cli/data.rs Outdated Show resolved Hide resolved
@Fumuran Fumuran force-pushed the andrew-support-pmt-via-cli-inputs branch 2 times, most recently from f7a1f5c to 17baffb Compare September 23, 2023 14:16
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left one nit inline. Once it is addressed, we can merge.

Comment on lines 5 to 7
# get the value `20` from the Partial Merkle Tree
push.0.2
mtree_get
Copy link
Contributor

Choose a reason for hiding this comment

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

For this and other cases below, I'd do the following:

  • Update the comment to say something like: "get value at depth=2, index=0 from the Partial Merkle Tree; this value should be 20"
  • Add assertions after mtree_get to verify that the value retrieved was indeed 20.

@Fumuran Fumuran force-pushed the andrew-support-pmt-via-cli-inputs branch from 17baffb to 765c532 Compare September 24, 2023 11:58
# push the root of the Merkle Tree on the stack
push.0x0463f7d47758ad94b11dbf9675ffb7b331baa9c150d7fac6d784055c313eab0e

# get value at depth = 2, index = 1 from the Merkle Tree; this value should be 22
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a typo here: shouldn't index also be 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Sorry, I missed it

@Fumuran Fumuran force-pushed the andrew-support-pmt-via-cli-inputs branch from 765c532 to 7548541 Compare September 24, 2023 23:17
@Fumuran
Copy link
Contributor Author

Fumuran commented Sep 24, 2023

Let's merge this PR after the change parse hex strings one, I want to be sure that this example is working.

@bobbinth
Copy link
Contributor

We could also merge now, and just make sure these tests work in the push PR.

@Fumuran Fumuran merged commit 9fba0ce into next Sep 25, 2023
15 checks passed
@Fumuran Fumuran deleted the andrew-support-pmt-via-cli-inputs branch September 25, 2023 11: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.

Support MerkleSet definitions via CLI .inputs file
2 participants