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.

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

chore: ensure that we can dial tls multiaddrs #1580

Merged
merged 4 commits into from
Sep 22, 2023

Conversation

danisharora099
Copy link
Collaborator

@danisharora099 danisharora099 commented Sep 19, 2023

Problem

js-waku is currently unable to dial a new format of secure websocket multiaddrs denoted by /tls/ws instead of wss
it was seen that js-libp2p supports these multiaddrs and that the bug actually came from an older version of @multiformats/multiaddr-to-uri which is specified in @libp2p/websockets

relevant issue on js-libp2p: libp2p/js-libp2p#2024

a PR was opened to upgrade the version of the faulty dependency which did not support tls format multiaddrs at the time but was discarded as the solution given was to update the lockfile: libp2p/js-libp2p#2059 (comment)

Solution

  • added a test for tls mutliaddrs
  • generated a fresh lock file

Notes

@github-actions
Copy link

github-actions bot commented Sep 19, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku core 27.69 KB (0%) 554 ms (0%) 471 ms (+7.4% 🔺) 1.1 s
Waku Simple Light Node 248.68 KB (-5.31% 🔽) 5 s (-5.31% 🔽) 1.1 s (+46.64% 🔺) 6 s
ECIES encryption 28.7 KB (0%) 574 ms (0%) 318 ms (+1.87% 🔺) 892 ms
Symmetric encryption 28.7 KB (0%) 575 ms (0%) 460 ms (+72.69% 🔺) 1.1 s
DNS discovery 110.62 KB (0%) 2.3 s (0%) 447 ms (-29.34% 🔽) 2.7 s
Privacy preserving protocols 117.02 KB (-0.32% 🔽) 2.4 s (-0.32% 🔽) 594 ms (-9.02% 🔽) 3 s
Light protocols 25.83 KB (0%) 517 ms (0%) 364 ms (-9.96% 🔽) 881 ms
History retrieval protocols 24.9 KB (0%) 499 ms (0%) 283 ms (-15.21% 🔽) 781 ms
Deterministic Message Hashing 5.65 KB (0%) 113 ms (0%) 74 ms (-45.07% 🔽) 187 ms

@danisharora099 danisharora099 force-pushed the chore/tls-multiaddr-test branch from d200013 to 65731f0 Compare September 19, 2023 15:49
@danisharora099 danisharora099 force-pushed the chore/tls-multiaddr-test branch from 13e51f9 to b9e7e9a Compare September 19, 2023 16:43
@danisharora099 danisharora099 marked this pull request as ready for review September 22, 2023 09:44
@danisharora099 danisharora099 requested a review from a team as a code owner September 22, 2023 09:44
// dummy multiaddr, doesn't have to be valid
await waku.dial(multiaddr(`/ip4/127.0.0.1/tcp/30303/tls/ws`));
} catch (error) {
if (error instanceof Error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it always instance of Error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not necessarily; Error is the most common way of throwing but one can throw literally anything

  • number: throw 404
  • boolean: throw false
  • string: throw "this is an error message"
  • object: {error: "this is an error message"}
  • ...

@weboko
Copy link
Collaborator

weboko commented Sep 22, 2023

with the old lockfile, it can be seen that when the test is added, it actually fails (https://github.com/waku-org/js-waku/actions/runs/6237429056/job/16931018217?pr=1580)

when a fresh lockfile is generated, the test passes https://github.com/waku-org/js-waku/actions/runs/6272635285/job/17034532746?pr=1580

I still didn't debug it but it seems that package-lock of old version resolves to old versions of packages and usually it happens when there are packages in monorepo that have different versions of the same package. Not sure if it is the case here but re-generating the lock is enough.

@danisharora099 danisharora099 merged commit a718c40 into master Sep 22, 2023
10 checks passed
@danisharora099 danisharora099 deleted the chore/tls-multiaddr-test branch September 22, 2023 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants