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

Various cleanups #91

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Various cleanups #91

wants to merge 4 commits into from

Conversation

daira
Copy link
Contributor

@daira daira commented Sep 29, 2022

  • Move orchard_vesta to zcash_test_vectors.orchard.vesta.
  • Fix errors due to the removal of RIPEMD-160 from default-available OpenSSL hashes.
  • Add a unit test runner.
  • Improve the usability of regenerate.sh and document it.

@@ -27,8 +29,16 @@ tv_scripts=(
zip_0244
zip_0316)

formats="${1:-rust json zcash}"

for generator in "${tv_scripts[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're fixing this, something that I frequently find myself doing is just regenerating a single set of test vectors, instead of regenerating them all. Now that we no longer use $2, maybe if that's present we could use that argument as the sole value in tv_scripts instead of regenerating everything?

Copy link
Contributor

Choose a reason for hiding this comment

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

The approach of using poetry run SCRIPT_NAME directly doesn't put the files in the expected place by itself.

Copy link
Contributor Author

@daira daira Sep 30, 2022

Choose a reason for hiding this comment

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

The (corrected) script takes a list of formats, so there would be a conflict unless it implemented more complicated argument parsing. You can always write poetry run SCRIPT_NAME -t FORMAT >OUTPUT_FILE, although I realize that's less convenient.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. Hm. I guess I'll just have to keep commenting out bits of this script when I need to regenerate stuff.

Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

Now that this has a test runner, that should be added to the CI configuration. Otherwise LGTM; I have not inspected the ripemd160 implementation.

regenerate.sh Show resolved Hide resolved
@daira
Copy link
Contributor Author

daira commented Oct 1, 2022

This broke the test vector regeneration test. Will fix.

@daira daira marked this pull request as draft March 21, 2023 21:23
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