feat(voice)!: add new encryption methods, remove old methods #10451
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
nyapat
force-pushed
the
feat/voice-encryption
branch
2 times, most recently
from
47606c3
to
4879a69
Compare
|
when #10472 is ready i will update tests for this (tweetnacl is being swapped out for stablelib & jest utterly hates it) |
nyapat
force-pushed
the
feat/voice-encryption
branch
from
52469d5
to
846b594
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10451 +/- ##
==========================================
- Coverage 38.00% 37.81% -0.20%
==========================================
Files 239 239
Lines 15488 15472 -16
Branches 1367 1351 -16
==========================================
- Hits 5886 5850 -36
- Misses 9587 9607 +20
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
nyapat
changed the title
| }; | ||
|
|
||
| void (async () => { | ||
| for (const libName of Object.keys(libs) as (keyof typeof libs)[]) { | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires | ||
| const lib = require(libName); | ||
| const lib = await import(libName); |
There was a problem hiding this comment.
Any particular reason for this change? Do we have any way to not require this to become async (as it can come with its own can of worms)... Maybe we should make this secretbox module a lazy loaded thing thats awaitable (export the two functions that handle imports themselves the first time)
There was a problem hiding this comment.
I'm not even sure how the require worked before? What was tsup compiling it to in ESM builds? There's no sync equivalent, which makes me think this just like, straight up didn't work for pure ESM users.
There was a problem hiding this comment.
You can create the require function [in ESM] by using createRequire() from node:module
There was a problem hiding this comment.
Realistically its still something we ought to change, this random void Promise thing is hella janky imo (and prone to race conditions or worse)
There was a problem hiding this comment.
i did find an alternative that is fine with cjs require, should i just swap over to that and revert this change to import?
There was a problem hiding this comment.
sorry to be annoying with notifs ^_^ but ^if i swap the dependency to a cjs one (and thus go back to require) does this solve the issue? will do if so!
There was a problem hiding this comment.
I mean you didn't show what the alternative is 😅, so 🤷♂️
There was a problem hiding this comment.
okok, it's @noble/ciphers unless you have any other suggestions
There was a problem hiding this comment.
Sure! But would love the thoughts of @didinele and/or @discordjs/core about this too (if we should just leave it as is or not)
Please describe the changes this PR makes and why it should be merged:
Added the new encryption methods that Discord will enforce for all voice users in November. Since the older methods are being deprecated entirely, I am removing them as well. Because tweetnacl does not support the cipher methods necessary for xchacha, I have removed it and replaced it with @stablelib/xchacha20poly1305 (aes256gcm does not require anything other than node:crypto, so that could theoretically be the only one needed if the vc always requested that)
Testing has been added for the encryption methods aswell, and I have changed some older tests to properly remove the RTP header based on the original encrypted data.
Status and versioning classification: