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

refactor(verification): organization and typing improvements [part 1/9] #830

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Oct 27, 2023

Motivation

This PR is the first one in a series of 9 PRs that finishes the vertex verification refactors. The objective is to completely remove all inheritance from the verification process, removing implicit dependencies between verifiers, streamlining the verification process and improving clarity on the scope/origin of each verification method, all to improve code readability, maintainability, and patching (for the simulator).

This is done incrementally. In previous PRs, we moved the verification out of the model classes. Now, in this series, we'll first change inheritance to composition, and then completely remove the composition too, leaving each verifier with the least possible amount of dependencies.

Also, in this PR, we install the typing_extensions dev dependency. This is a project from the official Python organization, and what it does is enable use of new type system features on older Python versions. In other words, it simply allows us to use features from the typing module that are not available in all Python versions we support.

Specifically, for this refactor we'll use assert_never(), a function to be used were we would normally use something like assert False, but that can be correctly type-checked by mypy. This improves robustness of match statements for example, that can be made exhaustive with this. We also use the @override annotation in next PRs, improving robustness and safety guarantees of inheritance. We may also find other useful typing features in the future that wouldn't be available otherwise.

Acceptance Criteria

  • Change BaseTransaction and all subclasses to use TxVersion as the version type, instead of int.
  • Install the typing_extensions dev dependency.
  • Move verify_unsigned_skip_pow() from TransactionVerifier to create_tx.py. This resolves Move verify_unsigned_skip_pow() #825.
  • In VerificationService:
    • Change all isinstance() checks to type() is, which is more restrictive.
    • Change raise NotImplementedError usages in match statements to assert_never(), which can be checked by the static type checker.
    • Remove validate_vertex_error(), inlining it HathorManager.
  • Set self.verifiers variable in test_verification, updating all usages. This will be useful in the next PRs.
  • No behavior should be changed by this PR.

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@glevco glevco self-assigned this Oct 27, 2023
@glevco glevco changed the title refactor(verification): organization and typing improvements refactor(verification): organization and typing improvements [part 1/9] Oct 27, 2023
@glevco glevco marked this pull request as ready for review November 1, 2023 15:54
msbrogli
msbrogli previously approved these changes Nov 1, 2023
pyproject.toml Outdated Show resolved Hide resolved
hathor/verification/verification_service.py Show resolved Hide resolved
msbrogli
msbrogli previously approved these changes Nov 3, 2023
jansegre
jansegre previously approved these changes Nov 6, 2023
@glevco glevco dismissed stale reviews from jansegre and msbrogli via 9c7038b November 6, 2023 23:49
@glevco glevco force-pushed the refactor/verification-inheritance/1-improvements branch from 6d844b9 to 9c7038b Compare November 6, 2023 23:49
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (8671304) 85.18% compared to head (c632ffe) 85.19%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #830   +/-   ##
=======================================
  Coverage   85.18%   85.19%           
=======================================
  Files         285      285           
  Lines       22655    22652    -3     
  Branches     3444     3443    -1     
=======================================
- Hits        19299    19298    -1     
- Misses       2671     2673    +2     
+ Partials      685      681    -4     
Files Coverage Δ
hathor/transaction/base_transaction.py 96.13% <ø> (ø)
hathor/transaction/block.py 94.52% <ø> (ø)
hathor/transaction/merge_mined_block.py 72.97% <ø> (ø)
hathor/transaction/resources/create_tx.py 93.75% <100.00%> (+1.29%) ⬆️
hathor/transaction/token_creation_tx.py 99.11% <ø> (ø)
hathor/transaction/transaction.py 94.76% <ø> (ø)
hathor/verification/transaction_verifier.py 91.54% <ø> (-0.51%) ⬇️
hathor/manager.py 82.21% <50.00%> (+0.02%) ⬆️
hathor/verification/verification_service.py 89.77% <81.25%> (-0.66%) ⬇️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@glevco glevco force-pushed the refactor/verification-inheritance/1-improvements branch from 9c7038b to 8d48b50 Compare November 7, 2023 01:58
msbrogli
msbrogli previously approved these changes Nov 7, 2023
jansegre
jansegre previously approved these changes Nov 7, 2023
pyproject.toml Outdated Show resolved Hide resolved
@glevco glevco dismissed stale reviews from jansegre and msbrogli via 99af497 November 7, 2023 14:38
@glevco glevco force-pushed the refactor/verification-inheritance/1-improvements branch from 99af497 to 50281e7 Compare November 7, 2023 16:22
@glevco glevco force-pushed the refactor/verification-inheritance/1-improvements branch from 50281e7 to c632ffe Compare November 7, 2023 21:31
@glevco glevco merged commit f528a06 into master Nov 7, 2023
9 checks passed
@glevco glevco deleted the refactor/verification-inheritance/1-improvements branch November 7, 2023 22:26
@jansegre jansegre mentioned this pull request Nov 13, 2023
2 tasks
This was referenced Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Move verify_unsigned_skip_pow()
3 participants