Error handling improvements. #216
Conversation
timoxley
force-pushed
the
cherry-picked-improvements
branch
4 times, most recently
from
3ff1833 to
af9e903
Compare
timoxley
force-pushed
the
cherry-picked-improvements
branch
2 times, most recently
from
aaf9a6c to
21b3e6c
Compare
…wants ethereum address.
…ort set('path.value').
…setting up key exchange.
Avoid suppressed errors in Subscription pipeline.
…GroupKey. Fixes streamr-client-testing subscriber.js.
timoxley
force-pushed
the
cherry-picked-improvements
branch
from
21b3e6c to
9f0dbb1
Compare
… 10m to 3m. Sometimes the late subscribers will miss a message.
| */ | ||
|
|
||
| static decryptStreamMessage(streamMessage, groupKey) { | ||
| static decryptStreamMessage(streamMessage: MessageLayer.StreamMessage, groupKey: GroupKey) { |
There was a problem hiding this comment.
The static methods which take a non-null groupKey as a parameter could be instance methods of GroupKey class? That way this this EncryptionUtilBase would focus on abstract publicKey/privateKey handling.
There was a problem hiding this comment.
Another leftover from previous implementation.
They could definitely live elsewhere agreed, though not sure if they belong on GroupKey. This logic is implementation detail for the stream message encryption/decryption process, rather than anything specific to the GroupKey data. Adding this to GroupKey would make GroupKey aware of StreamMessage.
I'd prefer to have the GroupKey only knowing about GroupKey stuff, keep it as close to a self-contained data container as possible avoid slipping into "Fat Model" architecture. i.e. attach cross-model logic to features rather than datatypes.
There was a problem hiding this comment.
Yeah, not good to couple components together too much.
One possibility would be to create a separate StreamMessageEncryptor which would know StreamMessage and GroupKey.
Or maybe that method could be in StreamMessage, if we create an encryptor interface. StreamMessage could know that, but not the actual implementing GroupKeyEncryptor.
| constructor(options: StreamrClientOptions = {}, connection?: StreamrConnection) { | ||
| super() | ||
| this.id = counterId(`${this.constructor.name}:${uid}`) | ||
| this.id = counterId(`${this.constructor.name}:${uid}${options.id || ''}`) |
There was a problem hiding this comment.
If the use provides the id, it is maybe unique enough to distinguish the instances? This counterId(${this.constructor.name}:${uid}) could be just a fallback if no explicit id is provided.
There was a problem hiding this comment.
Good point, though the wrapper around the id here was added to ensure I get unique results across tests, but I can give the tests known "ids":
e.g.
const publisher = new Client({id: 'publisher'})
const subscriber = new Client({id: 'subscriber'})Doesn't matter so much as this is 100% for debugging/logging purposes currently, but maybe id isn't the right option property, perhaps it should be name or something.
There was a problem hiding this comment.
Yes, name would describe it better 👍
There is the same situation in Subscription class (line 53)
| groupKeyData: Uint8Array, | ||
| } | ||
|
|
||
| function GroupKeyObjectFromProps(data: GroupKeyProps | GroupKeyObject) { |
There was a problem hiding this comment.
Is either GroupKeyObject or GroupKeyProps a deprecated form or group key definition object? Or do we need both for other reasons?
There was a problem hiding this comment.
I don't recall the details but the object is in different shapes depending on the source.
There was a problem hiding this comment.
If the are equal variants I could even name those types as GroupKeyDefinition1 and GroupKeyDefinition2. But doesn't make big difference, any naming is fine.
| const cancelTask = sub.cancel() | ||
| sub = undefined | ||
| await cancelTask() | ||
| await cancelTask |
There was a problem hiding this comment.
Should this same fix be applied to line 288, where we currently do await cancelTask()?
There was a problem hiding this comment.
Yes, 100% correct, good spotting.
…class, just use GroupKey.validate().
Cherry-picked a bunch of incidental changes/fixes made in the process of creating #204 and #205 so the changes in those PRs can be redone on top of this and reviewed + applied cleanly.
Many of these changes, mainly tests, TS conversions, improving error handling & other mild refactorings were made in the process of trying to track down the unhandled rejection issue I had last week.