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 the manual eq impl on sapling::ShieldedData<PerSpend> #1989

Merged

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Apr 7, 2021

Motivation

Our initial implementation of eq on sapling::ShieldedData was missing some fields. These tests check that we are comparing equality for all the fields. And they make sure that swapping the first spend and output still compares equal.

Solution

  • Test that swapping the first spend and output on an arbitrary ShieldedData<PerSpend> compares equal
  • Test that replacing all the current fields on an arbitrary ShieldedData<PerSpend> compares equal

The code in this pull request has:

  • Documentation Comments

Review

@oxarbitrage can review this PR. It blocks testing #1829.

Related Issues

Extra tests for #1946.

Follow Up Work

Implement and test #1829.

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement C-cleanup Category: This is a cleanup NU-5 Network Upgrade: NU5 specific tasks P-Medium labels Apr 7, 2021
@teor2345 teor2345 added this to the 2021 Sprint 7 milestone Apr 7, 2021
@teor2345 teor2345 self-assigned this Apr 7, 2021
Copy link
Contributor

@oxarbitrage oxarbitrage 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.

@oxarbitrage oxarbitrage merged commit f8094cd into ZcashFoundation:main Apr 7, 2021
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 C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants