-
-
Notifications
You must be signed in to change notification settings - Fork 287
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
msgIdToStrFn: support Uint8Array #5240
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -27,8 +27,7 @@ export function fastMsgIdFn(rpcMsg: RPC.IMessage): string { | |||
} | |||
|
|||
export function msgIdToStrFn(msgId: Uint8Array): string { | |||
// eslint-disable-next-line @typescript-eslint/no-unsafe-return, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call | |||
return Buffer.prototype.toString.call(msgId, "base64"); | |||
return toHex(msgId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there any potential side-effects or considerations changing the return value from base64
to hex
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously we choosed base64
probably because of the memory efficiency, but it's an issue with Uint8Array anyway. Switching to hex
helps us:
- fix the issue with Uint8Array by using
toHex
function - have same message id format to other clients which make it easier to debug, for example Nimbus
I think the memory difference is no much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me, thanks for explaining 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toHex
util uses Buffer toString which is memory efficient too. Prefixing with "'0x" adds non-trivial unnecessary memory, but it's not that bad, and all hex strings are assumed to be prefixed so
🎉 This PR is included in v1.7.0 🎉 |
Motivation
IWANT
messages, however Lodestar missed some of themDescription
toHex()
utility which support both Buffer and Uint8Arraypart of #5074
part of ChainSafe/js-libp2p-gossipsub#411