-
Notifications
You must be signed in to change notification settings - Fork 107
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
change(chain): Refactor the handling of height differences #6330
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6330 +/- ##
==========================================
+ Coverage 77.69% 77.74% +0.05%
==========================================
Files 304 304
Lines 39585 39639 +54
==========================================
+ Hits 30756 30818 +62
+ Misses 8829 8821 -8 |
Sub
and Add
for Height
Sub
and Add
trait impls for Height
Tests for GBT RPCs are failing. I forgot to run them locally. I'm working on that. |
Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks excellent!
Feel free to ignore the couple trivial suggestions.
Co-authored-by: Arya <[email protected]>
@upbqdn is going to be out for this week. Do we want to do anything with this PR in the meantime or should it wait until he gets back? |
@arya2 and I have enough context to finish it off, the remaining comments are mainly tweaks. I think it would be good to finish it to avoid merge conflicts with other work. |
@arya2 this is ready for a re-review, the changes were a bit more substantial than I thought, but they helped remove a lot of code that's tricky to review. I can do a video review if that would help? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
I left a couple optional suggestions, feel free to ignore either or both of them.
Motivation
Close #6279.
Solution
This PR handles height differences similarly to
chrono::DateTime
andchrono::Duration
.Reviewer Checklist