Skip to content

refactor(verification): move vertex verification to its own service #779

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

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Sep 22, 2023

Motivation

This PR is the first one on a series of refactors related to vertex verification, aiming to move all verification related code out of model classes and to their own files, improving code modularity and enabling other future improvements related to injection of dependencies, patching, etc.

The 4 main entrypoint methods used for validation/verification (detailed below) were moved to a new class and modules. Now, instead of using inheritance, we implement concrete methods and functions that are explicit about from where each implementation is coming from. This also allows to remove unused arguments from method signatures, as they're not overrides of the same method anymore. Verification of each vertex type accepts a specific set of arguments. Other than this change of structure, no behavior should be changed by this PR, it's only a refactor moving code.

In the next PRs, we'll move the remaining verification methods.

Acceptance Criteria

  • Create new VerificationService class and (block|transaction|token_creation_transaction)_verification modules.
  • Move BaseTransaction methods to VerificationService. The only change in them is adding * to make the signatures more explicit.
    • Move validate_basic() and validate_full() concrete methods as-is.
    • Move verify_basic() and verify() abstract methods, making them concrete.
    • Move validate_tx_error() renaming it to validate_vertex_error().
  • Move Block, Transaction and TokenCreationTransaction methods verify_basic() and verify() to each respective *_verification module. The only change is also adding * to their signatures.
  • Update HathorManager to use a VerificationService, and update Builder and CliBuilder accordingly.
  • Update all usages of verification to use the new VerificationService instead of calling verification methods directly on vertices, including in tests.

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 Sep 22, 2023
@glevco glevco force-pushed the refactor/verification-service branch 2 times, most recently from f0e959d to 302f5aa Compare September 22, 2023 15:41
Base automatically changed from chore/drop-python-3.9 to master September 22, 2023 17:01
@glevco glevco force-pushed the refactor/verification-service branch from 302f5aa to 9a6928d Compare September 22, 2023 19:13
@glevco glevco marked this pull request as ready for review September 22, 2023 19:40
@glevco glevco force-pushed the refactor/verification-service branch from 9a6928d to 4ae614a Compare September 22, 2023 23:10
@glevco glevco force-pushed the refactor/verification-service branch from 4ae614a to 1121a3c Compare September 25, 2023 19:32
@glevco glevco force-pushed the refactor/verification-service branch from 1121a3c to fe3c2e9 Compare September 25, 2023 22:04
@jansegre jansegre merged commit a1cd450 into master Sep 26, 2023
@jansegre jansegre deleted the refactor/verification-service branch September 26, 2023 11:53
This was referenced Oct 17, 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.

3 participants