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

Test Eq/PartialEq for orchard keys #2187

Merged
merged 6 commits into from
May 26, 2021
Merged

Test Eq/PartialEq for orchard keys #2187

merged 6 commits into from
May 26, 2021

Conversation

dconnolly
Copy link
Contributor

@dconnolly dconnolly commented May 24, 2021

Motivation

The new ConstantTimeEq/PartialEq/Eq impls for keys pushed in #2184 lacked some tests/coverage.

Solution

Add some assertions in the generate_keys test to exercise those trait implementations.

The code in this pull request has:

  • Documentation Comments
  • Unit Tests and Property Tests

Review

Anyone, not urgent.

Related Issues

#2184

Follow Up Work

@dconnolly dconnolly requested a review from a team May 24, 2021 01:00
@dconnolly dconnolly added A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup P-Low labels May 24, 2021
@dconnolly dconnolly added this to the 2021 Sprint 10 milestone May 24, 2021
Copy link
Contributor

@teor2345 teor2345 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!

It might be helpful to add comments explaining why we don't test equality for:

  • spend_validating_key
  • full_viewing_key
  • diversifier_key
  • diversifier
  • _transmission_key

@dconnolly dconnolly requested a review from teor2345 May 25, 2021 16:41
@dconnolly dconnolly enabled auto-merge (squash) May 25, 2021 17:49
Copy link
Contributor

@teor2345 teor2345 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!

@dconnolly dconnolly merged commit 7894cec into main May 26, 2021
@dconnolly dconnolly deleted the ct-eq-tests branch May 26, 2021 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants