This repository has been archived by the owner on Jun 19, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix: Fetch local fingerprint from SDP #109
fix: Fetch local fingerprint from SDP #109
Changes from all commits
e08afbe
9cd5ce1
3c87add
e2e0c72
cd747c1
a426e13
57dd380
6261cc0
2a481e4
777b16a
def274b
7d1561f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 how this is handled in the js-libp2p codebase in general. Should we still use
localCert.getFingerprints
in case the API is available (e.g. on Chrome), or should we always use the hack through the SDP?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 would prefer the single code path across browsers.
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 think I would prefer trying to use the best option and falling back on the hack. But not blocking this.
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 agree with Marco here, but it would be good to know (debug logs) which option was used if we're going to have different paths.
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.
@SgtPooki I've added the fallback to fetching from the SDP as requested.
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.
we should add a negative test here to validate the fallback behaviour
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.
this just tests the regex behaviour. While there are no explicit tests for the fallback behaviour, it gets tested when the tests are run in Firefox as part of CI.