-
Notifications
You must be signed in to change notification settings - Fork 221
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
Simplify event subscription and fix serialization issue (#278) #279
Conversation
- rename Event -> TMEventData and - introduce ResultEvent type which will replace JsonRPCBlockResult, JsonRPCTransactionResult
- does this pass the integration test?
a8e0737
to
ff97e71
Compare
Codecov Report
@@ Coverage Diff @@
## master #279 +/- ##
========================================
+ Coverage 26.9% 27.0% +0.1%
========================================
Files 105 105
Lines 3844 3827 -17
Branches 1217 1215 -2
========================================
Hits 1037 1037
+ Misses 1963 1946 -17
Partials 844 844
Continue to review full report at Codecov.
|
52b6ce4
to
701f3f8
Compare
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.
👁 🔑 🕑 ✍
- export ResultEvent fields for consumption by caller
- instead return None if we didn't read a event (but e.g. a ping message)
- instead return None if we didn't read a event (but e.g. a ping message)
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.
🚅 ⛰ 🔭 🔋
I'll merged this as is but I still think this still needs some more work. But instead of re-iterating on the implementation only, it would be better to write up an ADR describing the event subscription and the made and to be made design choices briefly. I'll followup on that shortly. |
ref #278 and followup to #225