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

Move all contextual validation code into its own function #1313

Merged
merged 1 commit into from
Nov 17, 2020

Conversation

teor2345
Copy link
Contributor

Motivation

The contextual validation function is hard to test.

It also directly accesses the sled state, but we want to work on sled and contextual validation at the same time.

Solution

Move the contextual validation function into the check module, and call it via a wrapper in the state service.

This change has two benefits:

  • reduces conflicts with the sled refactor and any replacement
  • allows the function to be called independently for testing

The code in this pull request has:

  • Documentation Comments
  • Unit Tests and Property Tests
    • This code change is a basic refactor which enables future tests

Review

@yaahc is also working on a sled refactor right now, they might conflict.

Follow Up Work

Do the rest of the difficulty contextual validation #802

This change has two benefits:
* reduces conflicts with the sled refactor and any replacement
* allows the function to be called independently for testing
@teor2345 teor2345 added A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup labels Nov 17, 2020
@teor2345 teor2345 added this to the First Alpha Release milestone Nov 17, 2020
@teor2345 teor2345 requested a review from yaahc November 17, 2020 01:00
@teor2345 teor2345 self-assigned this Nov 17, 2020
@teor2345 teor2345 merged commit d7d1598 into ZcashFoundation:main Nov 17, 2020
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