Skip to content

Conversation

@jameshfisher
Copy link
Contributor

No, it doesn't have tests. Yes, it should have tests.

Copy link
Contributor

@WillSewell WillSewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good. Let me know your thoughts on my comments @jameshfisher. I'm happy to act on them if you don't have the time. As you say, it's missing tests. I will have a go at adding some.

@jameshfisher
Copy link
Contributor Author

These tests should pass, but they seem to be broken by NodeJS breaking changes.

There are two NodeJS APIs for decoding buffers: TextDecoder and StringDecoder.

TextDecoder is not available until recent versions?

StringDecoder seems to change behavior:

on NodeJS version 10+, it works as expected, returning e.g. "\"Hello!\""
on NodeJS version <=9, it returns strings with decimal-encoded bytes, e.g. "34,72,101,108,108,111,33,34"?!

Copy link
Contributor

@WillSewell WillSewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's looking good. But it does sound like we'll need to test for new node versions first :(

In case you forget, I think this new functionality should be documented in the README as part of this PR.

Also, I see you moved isEncryptedChannel to util.js. I think you might have missed my PR also moves #90, which also moves channelSharedSecret. It's not a big deal, so I'll let you decide whether you want to merge or close it.

@WillSewell WillSewell force-pushed the end-to-end-encryption branch from 2a6fc53 to 459e378 Compare November 21, 2018 17:11
@WillSewell WillSewell force-pushed the end-to-end-encryption branch from b79b8ef to d697baf Compare November 26, 2018 10:15
@WillSewell WillSewell merged commit 870d83f into master Nov 26, 2018
@WillSewell WillSewell deleted the end-to-end-encryption branch November 26, 2018 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants