Conversation
Codecov Report
@@ Coverage Diff @@
## master #694 +/- ##
==========================================
- Coverage 84.70% 84.60% -0.10%
==========================================
Files 252 258 +6
Lines 21801 21783 -18
Branches 2954 2943 -11
==========================================
- Hits 18467 18430 -37
- Misses 2692 2715 +23
+ Partials 642 638 -4
|
d1f4193 to
02a3548
Compare
516de72 to
3922d10
Compare
This was referenced Jul 11, 2023
02a3548 to
67e0848
Compare
3425778 to
022e889
Compare
glevco
commented
Jul 12, 2023
glevco
commented
Jul 12, 2023
022e889 to
a67a277
Compare
d80cc56 to
dbbd3b6
Compare
jansegre
previously approved these changes
Jul 26, 2023
dbbd3b6 to
4406f1b
Compare
jansegre
previously approved these changes
Jul 27, 2023
4406f1b to
1ba3af5
Compare
msbrogli
requested changes
Aug 4, 2023
1ba3af5 to
ddcde10
Compare
msbrogli
previously approved these changes
Aug 21, 2023
ddcde10 to
aa53857
Compare
msbrogli
previously approved these changes
Aug 22, 2023
jansegre
previously approved these changes
Aug 22, 2023
d7fa5a4 to
b7fc247
Compare
b7fc247 to
3561317
Compare
3561317 to
78229b7
Compare
jansegre
approved these changes
Aug 29, 2023
msbrogli
approved these changes
Aug 29, 2023
This was referenced Aug 29, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This PR resolves #552. There are 3 goals:
MemoryReactorClockinstead of aClock, so it's compatible with the reactor used in production (that is, it implements theIReactorCore,IReactorTime, andIReactorTCPTwisted interfaces.Clockonly implementsIReactorTime)IReactorCoreThis was done by leveraging Python protocols to represent an intersection of Twisted interfaces, which allows us to better represent the reactor type. By doing this, we were able to change tests to use a reactor that is more similar to the one used in production, as previously we were using a
Clock, that behaved differently than the production reactor during full node initialization. This also allows for simplifyingPubSub, as it had a specifc implementation for dealing withClock.Acceptance Criteria
ReactorCoreProtocol,ReactorTimeProtocol, andReactorTCPProtocolclasses stubbing the respective Twisted interfaces,IReactorCore,IReactorTime, andIReactorTCPReactorProtocolthat implements the 3 protocols above, effectively creating a type that represents an intersection of Twisted interfaces, and cast the Twisted reactor to that protocol (this resolves aXXXcomment inhathor/util.py)PubSubto improve its code, also removing support for "non-running" reactors (Clock)ReactorThreadenum as it's not used anymoreClocktoMemoryReactorClockin tests (this resolves aXXXcomment)HeapClocktoMemoryReactorHeapClockon simulatorverified_cast()utils function and move it to its own fileChecklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged