Feat/send cut heights #5
Conversation
|
The original decision to use cw-node heights came after a discussion about the appropriate API to query cw-data's "cut", which there was none. cw-node was used instead, but this already caused issues when querying a cw-stream-server that relied on a stalled cw-data. I worked around the lack of an API by polling Including coinbase means this can be relied upon to track even idle chains, like testnet04 can be at times. Some rejected approaches:
So we query: |
| export default class ChainwebDataHeightTracker { | ||
| private _maxHeight: number = -1; | ||
| private _running: boolean = false; | ||
| private _logger = new Logger('ChainwebDataHeightTracker'); |
There was a problem hiding this comment.
I see you use a custom logger solution. In general we use debug in other packages. Worth considering.
There was a problem hiding this comment.
We can discuss this further later on but as a note for this decision: I looked for a bit more advanced functionality in a few common packages (including debug) but after not finding something off the shelf that fit my requirements I built a small custom thing over npmlog
The things I was looking for:
- prefix log line with timestamp (now customizable/optional)
- prefix log with "section/module" that emitted that log line
- easily configurable log levels (error/warn/log/debug)
| registerUpdateCallback(fn: NumericCallback) { | ||
| validateType(CLASS_NAME, 'registerCallback', fn, 'function'); | ||
| this._callbacks.add(fn); | ||
| } | ||
|
|
||
| unregisterUpdateCallback(fn: NumericCallback) { | ||
| validateType(CLASS_NAME, 'unregisterCallback', fn, 'function'); | ||
| if (this._callbacks.has(fn)) { | ||
| this._callbacks.delete(fn); | ||
| return; | ||
| } | ||
| this._logger.warn('Unregistering callback: not found; ignoring'); | ||
| } |
There was a problem hiding this comment.
Often, the register callback function also returns a function that can be used to unregister the callback. When dealing with callbacks for specific events, it is beneficial to extend from EventTarget as it handles many cases for Event Emitters.
Note: I see EventTargets' addEventListener doesn't return with an unregister function though 😔
There was a problem hiding this comment.
I don't recall encountering the "register returns unregister" pattern - and we would have to store the unregister callbacks on the other consumer-side of these, so I'm not sure it would be preferable to using the "callback-as-identity" pattern in this specific use case. If we had multiple registrations of the same callback, it would be different.
For now I have opted for explicit register/unregister callback methods, but yes, the register/unregister callbacks can indeed be changed, e.g. these could extend EventEmitter2
src/sse/route-service.ts
Outdated
| const heightTracker = new ChainwebDataHeightTracker(); | ||
| heightTracker.start(); | ||
|
|
||
| const { network, confirmationDepth, heartbeatInterval } = config; |
There was a problem hiding this comment.
I'm not sure if starting the tracker in the root of the file is best practice. A solution could be to create a singleton getter and do initialisation there.
src/sse/route-service.ts
Outdated
| import ChainwebEventService from './chainweb-event.js'; | ||
| import Logger from './logger.js'; | ||
| import ChainwebCutService from './chainweb-cut.js'; |
There was a problem hiding this comment.
Side note: I see you have a lot of export defaults, it's considered best practice to only export named exports. This improves code readability and refactoring/navigation as the name stays the same across files.
There was a problem hiding this comment.
I'll leave this as a TODO as we want to expedite releasing
Implemented a new event
heights:this sends an event
heightswith payload:array of the current cut as seen on chainweb-nodemax height seen on chainweb-data. Structure:{ "data": <numeric height> }Needed to implement better reconnections. The corresponding client PR will use this value to resume with better guarantees of event delivery.
Update: A timely cw-data outage showed that we should not rely on the cw-node cut, so switching to getting the max height seen on cw-data and using that instead.
cw-node cut may be put back in the future if it proves useful.