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(sync-v2): improve deferreds handling #732

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Jul 26, 2023

Motivation

Simplify deferred-handling code by removing repetition and improving typing robustness.

Acceptance Criteria

  • Remove usage of string keys for "singular" deferreds (tips, best-block, and peer-block-hashes), by using a specific, typed attribute for each of them, instead of storing them in a generic dictionary.
  • By doing that, the deferreds dictionary is now only used for transactions, so it has been renamed and typed accordingly. The key type has also been changed to VertexId, so the origin is removed from it.
  • Change None handling in get_tx() from if/raise to assert, as the typing guarantees tx will never be None.

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 Jul 26, 2023
@glevco glevco marked this pull request as ready for review July 26, 2023 19:37
@glevco glevco requested review from jansegre and msbrogli as code owners July 26, 2023 19:37
jansegre
jansegre previously approved these changes Jul 26, 2023
hathor/p2p/sync_v2/manager.py Outdated Show resolved Hide resolved
hathor/p2p/sync_v2/manager.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #732 (a54f623) into master (405ab6a) will decrease coverage by 0.04%.
The diff coverage is 86.20%.

❗ Current head a54f623 differs from pull request most recent head b602039. Consider uploading reports for the commit b602039 to get more accurate results

@@            Coverage Diff             @@
##           master     #732      +/-   ##
==========================================
- Coverage   84.39%   84.35%   -0.04%     
==========================================
  Files         251      251              
  Lines       21586    21577       -9     
  Branches     2917     2916       -1     
==========================================
- Hits        18218    18202      -16     
- Misses       2737     2742       +5     
- Partials      631      633       +2     
Files Changed Coverage Δ
hathor/p2p/sync_v2/manager.py 74.09% <86.20%> (-1.49%) ⬇️

... and 4 files with indirect coverage changes

@glevco glevco force-pushed the refactor/sync-v2/improve-deferreds branch from a54f623 to 25fe1cf Compare July 26, 2023 22:32
@glevco glevco force-pushed the refactor/sync-v2/improve-deferreds branch from 25fe1cf to b602039 Compare July 26, 2023 22:37
@msbrogli msbrogli merged commit b602039 into master Jul 26, 2023
@msbrogli msbrogli deleted the refactor/sync-v2/improve-deferreds branch July 26, 2023 22:38
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