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.

Sign up for GitHub

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

Jump to bottom

Set the ufrag/pwd according to the spec #2924

Merged
merged 7 commits into from
Oct 31, 2022
Merged

Set the ufrag/pwd according to the spec #2924

merged 7 commits into from
Oct 31, 2022

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Oct 24, 2022

Seeing the small discussion in the WebRTC implementation PR, I realized that smoldot doesn't actually say what is in the spec now. This PR changes it.

@tomaka tomaka requested a review from melekes October 24, 2022 11:34
mergify[bot]
mergify bot approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@mergify mergify bot left a comment

Choose a reason for hiding this comment

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

Automatically approving tomaka's pull requests. This auto-approval will be removed once more maintainers are active.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2022 •
edited
Loading

twiggy diff report

Difference in .wasm size before and after this pull request.

 Delta Bytes │ Item
─────────────┼──────────────────
          +0 ┊ Σ [0 Total Rows]

@tomaka
Copy link
Contributor Author

tomaka commented Oct 24, 2022 •
edited
Loading

Doesn't work on Chrome:

Failed to execute 'setLocalDescription' on 'RTCPeerConnection': Failed to set local offer sdp: Failed to apply the description for m= section with mid='0': Invalid ICE parameters: ICE ufrag must contain only alphanumeric characters, '+', and '/'.

I've commented accordingly in the spec.

@tomaka
Copy link
Contributor Author

tomaka commented Oct 24, 2022

Since Chrome throws an exception, it's clear that it's going to be changed again, and I'm not merging this PR beforehand.

@tomaka tomaka added the blocked Can't work on this issue because it is blocked on something out of our control label Oct 25, 2022
@tomaka tomaka added automerge Automatically merge pull request as soon as possible and removed blocked Can't work on this issue because it is blocked on something out of our control labels Oct 31, 2022
@tomaka
Copy link
Contributor Author

tomaka commented Oct 31, 2022

I'm going to merge this because I'm releasing a new version.
We can always open another PR if the spec changes again.

@mergify mergify bot merged commit fba43ad into paritytech:main Oct 31, 2022
@tomaka tomaka deleted the ufrag-pwd branch October 31, 2022 07:01
@tomaka tomaka mentioned this pull request Nov 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melekes melekes melekes approved these changes

@mergify Mergify Mergify approved these changes

Assignees
No one assigned
Labels
automerge Automatically merge pull request as soon as possible
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tomaka @melekes