Skip to content
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

fix: topicToKey should return a buffer not a string #16

Merged
merged 1 commit into from
Sep 25, 2019

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Sep 24, 2019

Also, Buffer.toString() without specifying an encoding can result in unprintable characters being part of the string which can result in false positives when comparing stringified buffers.

E.g:

Buffer.from([254]).toString() === Buffer.from([253]).toString()
// true

Buffer.from([254]).toString('hex') === Buffer.from([253]).toString('hex')
// false

Also, also, stop putting stringified buffers into the logs because it can corrupt your console output.

Also, `Buffer.toString()` can result in unprintable characters being
part of the string which can result in false positives when comparing
stringified buffers.

Also, also, stop putting stringified buffers into the logs because
it can corrupt your console output.
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM!

Can you just test this commit version with ipfs/interop tests before I release this?

@achingbrain
Copy link
Member Author

achingbrain commented Sep 25, 2019

Interesting, does the go implementation stringify buffers and use them for comparison? There have been bugs with this previously where it used the default ASCII encoding for byte arrays sent in HTTP responses which meant you then couldn't recreate them on the client side as it generated invalid JSON.

@vasco-santos
Copy link
Member

Yes, that was a problem at the time I implemented this. The topics needed to be in this exact format to be consistent with the go side

@achingbrain
Copy link
Member Author

Hmm, this sounds like a bug in the go implementation. We shouldn't replicate their bugs 🤣

@vasco-santos
Copy link
Member

Hmm, this sounds like a bug in the go implementation. We shouldn't replicate their bugs 🤣

Yes, except when we also need to interop with them! I would like a venue to have these discussions

@achingbrain
Copy link
Member Author

The interop tests with this pass for me locally. Need ipfs/js-ipfs#2428 to go in before it'll pass on CI though as it's got some fixes for other failing interop tests.

@vasco-santos
Copy link
Member

Cool! So, should I move and release this?

@achingbrain
Copy link
Member Author

Yes please!

@vasco-santos vasco-santos merged commit 354714f into master Sep 25, 2019
@vasco-santos vasco-santos deleted the key-should-be-a-buffer branch September 25, 2019 12:40
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.

2 participants