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

bulletproofs: Address test-code warnings #82

Merged
merged 8 commits into from
Aug 6, 2024
Merged

bulletproofs: Address test-code warnings #82

merged 8 commits into from
Aug 6, 2024

Conversation

rillian
Copy link
Contributor

@rillian rillian commented Jul 29, 2024

Address various issues with the bulletproofs code

  • Rewrite utils::add_vec to use iterators
  • Add some minimal tests for utils
  • Fix dead code warnings with annotations
  • Remove inactive profile and cfg_attr directives

rillian added 8 commits July 29, 2024 09:22
Silence this warning by annotating the functions. For a test
harness using something flat, matching the mathematical notation
is fine.
I think these were copied from the upstream crate source, but they
have no effect in the workspace environment we have here; only
the top-level Cargo.toml profiles have any effect.

Remove these since cargo 1.80 warns about them.
This warning in specific to clippy, so the bare directive has
no effect, but rustc 1.80 warns about it.
The rest of the code assumes the `std` library, and no such feature
is defined in Cargo.toml, so this has no effect. Furthermore,
I don't think the `error` annotation has any effect without an
import of the `thiserror` crate or similar. Which would be fine,
but isn't currently part of the build. The comment is sufficient
in the meantime.
Silence a dead code warning by marking the naive version of
`scalar_exp_vartime` test-only. Then add a test so there's
some coverage of the code.
Make these a little more formal and easier to follow.
Rewrite `utils::add_vec` to use iterators. This should generate
better code since it's easier to eliminate bounds checks.

Make the function private and replace the comment with an actual
assertion that the argument lengths match. This is only used
locally (so far) so it doesn't need to be accessible outside
the module.

Add some minimal test coverage.
@rillian rillian requested review from claucece and ankeleralph July 29, 2024 16:27
@rillian rillian self-assigned this Jul 29, 2024
Copy link
Collaborator

@ankeleralph ankeleralph 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 to me :)

@rillian rillian merged commit 2feaa76 into main Aug 6, 2024
4 of 5 checks passed
@rillian rillian deleted the warnings branch August 6, 2024 17:13
@rillian rillian mentioned this pull request Aug 8, 2024
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