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: create non-null hash property #967

Merged
merged 1 commit into from
Mar 19, 2024
Merged

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Mar 13, 2024

Motivation

Currently, the BaseTransaction.hash property is of type Optional[bytes], which is pretty inconvenient considering the hash has (and should have) a value most times. For that, we have to use asserts in multiple places to guarantee the value is not None.

This PR renames the existing optional property to _hash, creating a new calculated hash property that performs the assert for us. By doing it like this, every place that already uses the existing hash property will continue to work (only a few places that rely on the possibility of the value being None had to be updated).

Acceptance Criteria

  • Rename current optional BaseTransaction.hash property to _hash and add new non-null hash property.
  • Remove some unnecessary asserts.

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 Mar 13, 2024
@glevco glevco force-pushed the refactor/hash-property branch from 43eb06d to 5916646 Compare March 13, 2024 16:02
@glevco glevco marked this pull request as ready for review March 13, 2024 16:14
Copy link

codecov bot commented Mar 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.15%. Comparing base (a2ee438) to head (6c494e6).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #967      +/-   ##
==========================================
+ Coverage   85.05%   85.15%   +0.10%     
==========================================
  Files         294      294              
  Lines       22782    22783       +1     
  Branches     3428     3430       +2     
==========================================
+ Hits        19378    19402      +24     
+ Misses       2718     2702      -16     
+ Partials      686      679       -7     

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

@glevco glevco force-pushed the refactor/hash-property branch from 5916646 to 6c494e6 Compare March 19, 2024 16:28
@glevco glevco merged commit 8ed3e4b into master Mar 19, 2024
12 checks passed
@glevco glevco deleted the refactor/hash-property branch March 19, 2024 18:49
@jansegre jansegre mentioned this pull request Apr 5, 2024
2 tasks
@jansegre jansegre mentioned this pull request May 8, 2024
2 tasks
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