-
Notifications
You must be signed in to change notification settings - Fork 63
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
feat: don't throw on invalid b58 string in getPeerId #43
Conversation
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.
Not sure if this is an improvement.
Btw, this function should be documented.
src/index.js
Outdated
|
||
bs58.decode(b58str) | ||
return b58str |
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.
@dryajov this way you are silencing the error and returning null. Why do you think this is a better approach?
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.
I want the utility function to be more ergonomic, in the majority of cases you want to just ma.getPeerId()
from a multiaddr and not worry about wrapping it in a try/catch, if the id was missing or invalid, you'll get a null. The intent here is not to return an invalid b58 string and I think this accomplishes it?
src/index.js
Outdated
})[0][1] | ||
|
||
bs58.decode(b58str) | ||
} catch (e) {} |
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.
ah, I think you might be asking about this part here?
I should be clearing b58str in the catch, I'll fix that. Otherwise, I think returning null
on invalid/empty ids is an acceptable solutions, as long as no malformed ids are returned?
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.
LGTM
No description provided.