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

Derive Arbitrary impls for a bunch of chain and network types #2179

Merged
merged 2 commits into from
May 24, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented May 21, 2021

Motivation

We need Arbitrary impls to write proptests for internal and external network protocol messages.

Solution

  • Derive Arbitrary impls for a bunch of chain and network types
  • Add proptest Strategy impls for u32 and full datetimes

The code in this pull request has:

  • Documentation Comments
  • Enables Unit Tests and Property Tests

Review

Anyone can review, as long as they understand proptests.

Related Issues

Split off #2160

These tests might have caught security issue #2148

Follow Up Work

Rest of #2160

@teor2345 teor2345 added A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement P-Medium labels May 21, 2021
@teor2345 teor2345 requested a review from dconnolly May 21, 2021 02:39
@teor2345 teor2345 self-assigned this May 21, 2021
@teor2345
Copy link
Contributor Author

I think the macOS sync_large_checkpoints_mainnet failure was caused by #2172 or #2154, and it should be fixed by #2181 or #2182.

I don't think it's related to this PR, because this PR only changes proptests.

https://github.com/ZcashFoundation/zebra/pull/2179/checks?check_run_id=2635749551#step:10:644

@dconnolly
Copy link
Contributor

The zebra-chain::primitives module has been a place for cryptographic primitives such as the signature schemes and the proof types, rather than 'utils' or other base types. Sometime like Amount got its own module. Should we have a top-level arbitrary module for things like this?

Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

@teor2345 The zebra-chain::primitives module has been a place for cryptographic primitives such as the signature schemes and the proof types, rather than 'utils' or other base types. Sometime like Amount got its own module. Should we have a top-level arbitrary module for things like this?

@teor2345
Copy link
Contributor Author

teor2345 commented May 24, 2021

@teor2345 The zebra-chain::primitives module has been a place for cryptographic primitives such as the signature schemes and the proof types, rather than 'utils' or other base types. Sometime like Amount got its own module. Should we have a top-level arbitrary module for things like this?

These particular strategies are serialization-specific and consensus-critical, so I moved them to serialization::arbitrary.

@teor2345 teor2345 requested review from dconnolly and jvff May 24, 2021 08:11
@teor2345
Copy link
Contributor Author

@jvff this should be ready to merge if you want to use it in the #2178 tests

@teor2345 teor2345 enabled auto-merge (squash) May 24, 2021 08:14
@teor2345 teor2345 merged commit f0549b2 into main May 24, 2021
@teor2345 teor2345 deleted the extra-arbitrary branch May 24, 2021 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants