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

fix: fix externally defined webtransport addresses #2224

Closed
wants to merge 3 commits into from

Conversation

aschmahmann
Copy link
Collaborator

fixes #2223

Support externally defined (e.g. with externally known ports from port forwarding, dns, etc.) webtransport addresses. This mechanism is very hacky and should be removed as expediently as possible.

There are definitely better wasy to do this, but wanted to at least put out an option that exists so that:

  1. We at least have something we're looking at/comparing against
  2. There's code in case this fix is deemed "good enough" for now
  3. If downstream consumers feel they need this fix sooner than it can be delivered in go-libp2p they can copy this hack into their codebases

Maintainers can feel free to close or modify this PR.

Support externally defined (e.g. with externally known ports from port
forwarding, dns, etc.)  webtransport addresses. This mechanism is very
hacky and should be removed as expediently as possible.
@aschmahmann aschmahmann force-pushed the fix/hacky-addrfactory-webtransport branch from a3a2b8d to fa32519 Compare March 26, 2023 06:14
@SgtPooki
Copy link
Member

Can confirm this populates my certhash properly when adding

    "AppendAnnounce": [
      "/ip4/x.x.x.x/udp/4001/quic-v1/webtransport"
    ],

to my ~/.ipfs/config

@MarcoPolo MarcoPolo self-assigned this Mar 27, 2023
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! This looks correct to me, but it does bake in some assumptions which might not be true in the future (e.g. there'll always be exactly 2 certhash components).

We really need to refactor our API and get rid of this AddrsFactory. Having external applications (and internal components as well) mess with the addresses is not ideal...

@p-shahi
Copy link
Member

p-shahi commented Apr 3, 2023

Can we close this in favor of #2227 ?

@aschmahmann
Copy link
Collaborator Author

Yep, they're fairly equivalent and Marco has made more reusable exports in his. Thanks for pushing on this 🙏

@aschmahmann aschmahmann closed this Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants