Support for websocket connections to in-node versions of chronik
Details
Details
- Reviewers
Fabien tobias_ruck - Group Reviewers
Restricted Project - Commits
- rABC6cf4dcc28d26: [chronik-client] Support websockets in ChronikClientNode
See D14915 for instructions or wait for CI
npm test for new subscription validation function
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- cc-websocket
- Lint
Lint Errors Severity Location Code Message Error test/functional/setup_scripts/chronik-client_websocket.py:122 F841 flake8 F841 - Unit
No Test Coverage - Build Status
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 846 ↗ | (On Diff #45304) | ? seems this matches the proto, /** Error message returned from our APIs. */
export interface Error {
/** 2, as legacy chronik uses this for the message so we're still compatible. */
msg: string;
}what should it be? |
| modules/chronik-client/test/vectors.ts | ||
| 45 ↗ | (On Diff #45304) | JS users of the lib could still do this though right? |
| 80 ↗ | (On Diff #45304) | JS users of lib? |
| modules/chronik-client/test/vectors.ts | ||
|---|---|---|
| 80 ↗ | (On Diff #45304) | Touché. Then you have to coerce the type "madeitup" as any as "other" or so, or change isValidWsSubscription to allow for string inputs |
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 846 ↗ | (On Diff #45304) | |
Comment Actions
get npm test to work with types and coerced types, clean up other types, org suggestions from inline comments
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 419 ↗ | (On Diff #45317) | if (typeof msg.error !== 'undefined') {
this.onMessage({ type: 'Error', ...msg.error });
}@tobias_ruck is this right? I'm not sure how to get this from a test. modeling the implementation from nng chronik-client. |
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 419 ↗ | (On Diff #45317) | It's ok; probably better to just do this. NNG had more fields here, but no need to make it complicated if they aren't there. You can test it by sending anything that's currently caught by your validation, e.g. send a p2pkh subscription with anything other than 20 bytes of payload. At least that's what's supposed to happen. |
Comment Actions
separate blocks subunsub function
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 419 ↗ | (On Diff #45317) | If I back out the client-side validation and run ws.sub('p2pkh', '1111111111') -- this gets added to ws.subs and I get no error msg from the websocket. Would be a tough API to handle as an app dev either way, since the error in this case would not be thrown by the subscription, but arrive as a msg that needs to be listend for, specifically parsed, then the dev needs to know to unsub from the bad one. Prob the dev has some msg handler setup that is only looking for type: 'Tx' msgs, could be missed or misunderstood. So imo catching it client side before we even send the msg is preferable. Still, potential issue if expected behavior is to get the error as a msg from the ws. |
Comment Actions
If I back out the client-side validation and run ws.sub('p2pkh', '1111111111') -- this gets added to ws.subs and I get no error msg from the websocket.
Still, potential issue if expected behavior is to get the error as a msg from the ws.
This is fixed in D15472, there was an error sent from Chronik but it was encoded in the wrong msg.
So imo catching it client side before we even send the msg is preferable.
Yes, agreed. Ideally it would remove the sub from ws.subs, but I don't think we have that info available.
If we handle the error for the user (let'd do that), we should remove the Error from WsMsgClient sent via onMessage. Also, we might want to change onError to receive either the JS WebSocket error or the protobuf error from the Error message. This is because we might want to add more failure modes other than failed parsing in the future and apps being prepared to receive and handle those errors is an advantage. Also, our validation might not catch all cases so having some way to receive those errors is still an advantage.
Comment Actions
I can confirm we now get the expected error msgs from chronik, e.g.
{
"msg": "400: Invalid payload for P2PKH: Invalid length, expected 20 bytes but got 5 bytes"
"type": "Error"
}For now, the only instances that throw this error -- attempting to subscribe to invalid scripts -- are handled in front-end validation.
Also, we might want to change onError to receive either the JS WebSocket error or the protobuf error from the Error message.
I'm not sure how we could do this. Right now, a JS error would simply throw an error -- like what the client-side validation is already doing. Any server-side error being communicated to the client as a chronik msg seems fine -- and is handled already. I guess we could throw an error if we get this kind of msg from the server? But hard to say if that's really what the dev wants.
Leaving it as is -- i.e., the app dev gets a msg that is clearly an error msg and can figure out how to handle it, even if they are at the moment caught client-side -- probably the best approach for now.
Comment Actions
The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 320 ↗ | (On Diff #45346) | there will be subscribeToTokenId too and probably subscribeToPlugin, I think it's best to not make the script one special by being "the subscribe" |
| 390 ↗ | (On Diff #45346) | |
| 417 ↗ | (On Diff #45346) | what do you think about moving this to this.onError(msg.error)? Either way works for me |
| 768 ↗ | (On Diff #45346) | note: if you do move the error to onError, you can remove the error here; up to you |
| 820 ↗ | (On Diff #45346) | this comment needs to be fixed |
| 833 ↗ | (On Diff #45346) | this comment needs to be fixed |
| modules/chronik-client/test/integration/mempoolconflicts.ts | ||
| 1 ↗ | (On Diff #45346) | no copyright notice? they seem to be missing in a lot of files in this diff; calling my lawyer right now |
Comment Actions
separate functions for subunsub by what is being subscribed/unsubsrcibed to, improve comment, remove now-unused type
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 302 ↗ | (On Diff #45367) | The subUnsub* API is using a confusing double negation (to subscribe, you use isunsub = false). This is also inconsistent with the current NNG API, is there a rational behind this change ? |
| 377 ↗ | (On Diff #45367) | Any reason this is public (and exported) ? Apart from the above comment about the bool param, this also looks like something that should not be used directly by the calling app |
| 377 ↗ | (On Diff #45367) | Why is this public (and exported) ? This doesn't look like a method that should be called directly by the calling app |
| 390 ↗ | (On Diff #45367) | same |
| modules/chronik-client/src/validation.ts | ||
| 7 ↗ | (On Diff #45367) | +1 |
| modules/chronik-client/test/integration/websocket.ts | ||
| 13 ↗ | (On Diff #45367) | Why is that needed ? |
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 302 ↗ | (On Diff #45367) | It was changed in the in-node Chronik; the rationale was that protobuf has defaults if omitted and the default for a bool is false, and it seemed more natural for a subscription message to default to subscribe as opposed to unsubscribe. |
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 302 ↗ | (On Diff #45367) | OK |
Comment Actions
cleaning up based on feedback, move subunsub functions to private, move regexp generation out of validation function, line break
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 377 ↗ | (On Diff #45367) | oversight, fixed this was called by failoverProxy, so had to be public. But, no reason failoverProxy can't use the now-less-ambiguous subscribeToScript and subscribeToBlocks methods |
| 390 ↗ | (On Diff #45367) | oversight, fixed |
| 417 ↗ | (On Diff #45346) | It does seem like a better design. Lots of reasons onError method should be different from onMessage. However, without any reachable error events, it's purely hypothetical that this would be preferable in the future. imo should be its own diff, and should have clear impact to warrant a diff. Being untestable, I'm ok holding off. |
| 833 ↗ | (On Diff #45346) | now that all the subunsub functions are distinct, we don't need this combined type. not used anywhere else in the file but here. removed. |
| modules/chronik-client/test/integration/mempoolconflicts.ts | ||
| 1 ↗ | (On Diff #45346) | been a TODO for awhile now...need to handle this with a linter. |
| modules/chronik-client/test/integration/websocket.ts | ||
| 13 ↗ | (On Diff #45367) | using @ts-ignore will throw a compilation error without this. @ts-ignore is used two times in this file to test validation errors that would only be possible to experience for JS users of this lib. |
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 417 ↗ | (On Diff #45346) | I’m ok with the current design, it means we have a way for users to handle errors and if there’s testable errors in the future it won’t be a breaking change to start to emit those. |
| modules/chronik-client/test/integration/websocket.ts | ||
| 226 ↗ | (On Diff #45367) | If you want to avoid the ts-ignore comment you can do this |
Comment Actions
one last thing, otherwise lgtm
| modules/chronik-client/src/validation.ts | ||
|---|---|---|
| 7 ↗ | (On Diff #45402) | Another option: change the payout to lowercase before testing |
Comment Actions
rebase, version bump, clarify that hex payload strings must be lowercase and add tests
| modules/chronik-client/src/validation.ts | ||
|---|---|---|
| 7 ↗ | (On Diff #45402) | Existing hex conversion functions in src/hex.ts only support lowercase hex input. I looked at applying toLowerCase() before passing to the server -- this would support uppercase and mixed case user input. However, I think it is better to restrict everything to lowercase. In this way everything in this.subs is standard and equal to what the user subscribed to. I added unit tests confirming that uppercase and mixed case inputs are rejected and updated the error msg to reflect this. |