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

Document /http #550

Merged
merged 15 commits into from
Jul 10, 2023
Merged

Document /http #550

merged 15 commits into from
Jul 10, 2023

Conversation

MarcoPolo
Copy link
Contributor

@MarcoPolo MarcoPolo commented May 31, 2023

This is meant to clarify what libp2p is expecting of the /http component and avoid folks creating conflicting interpretations of the component. This purposely does not include anything libp2p+http (but hints at it) with the goal of clarifying expectations of the /http component early.

Please take a look:
@lidel
@marten-seemann
@willscott
@masih

This was prompted by ipni/go-libipni#42 + historical issues referenced in ipni/go-libipni#42 (comment)

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Currently, we expect to immediately run multistream-select after a connection is established. Connections are described by multiaddr.

How does this interact with the /http transport component?

I am guessing we don't run multistream-select on connections using the /http component?

@thomaseizinger
Copy link
Contributor

Currently, we expect to immediately run multistream-select after a connection is established.

Actually, that is not true for QUIC.

Anyway, it might be worth mentioning this briefly.

@MarcoPolo
Copy link
Contributor Author

We know this is an HTTP transport. Multistream-select is not used here. The specifics I believe belong in the libp2p+http doc since it's how libp2p uses the HTTP transport.

Copy link
Contributor

@BigLep BigLep 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 creating and getting this written down.

http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

With my limited knowledge of the libp2p+http effort, this looks good to me apart from the smaller comments below.

http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
@BigLep
Copy link
Contributor

BigLep commented Jun 1, 2023

Related issue: multiformats/multiaddr#63

@BigLep
Copy link
Contributor

BigLep commented Jun 1, 2023

This topic came up during the 2023-06-01 IPFS Implementers Working Group (recording should go live by end of week). Everyone was told to engage here. We expect to use the 2023-06-15 occurrence to close out on this (i.e., resolve anything that wasn't handled async). Anyone is welcome to join that call (details).

http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Outdated Show resolved Hide resolved
http/transport-component.md Show resolved Hide resolved
@BigLep
Copy link
Contributor

BigLep commented Jun 13, 2023

@MarcoPolo : I know you want to have more sync time during https://pl-strflt.notion.site/IPFS-Implementers-Sync-2023-06-15-b8556243125442f2b0bf48c5db90d056?pvs=4 . I assume you'll consolidate down the open questions and areas of disagreeement in advance.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

One question regarding the semantics of TLS, other than that, this is a really good document and I am looking forward to this!

http/transport-component.md Show resolved Hide resolved
@MarcoPolo
Copy link
Contributor Author

All outstanding concerns have been addressed. We brought this up in the ipfs implementers meeting and no objections were raised there either. I think we can merge this as soon as we get an approval.

@MarcoPolo MarcoPolo requested a review from marten-seemann June 16, 2023 19:44
@BigLep
Copy link
Contributor

BigLep commented Jul 3, 2023

What are the next steps for merging this? Are we waiting to also include #554 or is that going to be a separate PR?

@MarcoPolo
Copy link
Contributor Author

#554 Will happen in a separate PR. I'd like @marten-seemann to give his approval here before merging

http/transport-component.md Outdated Show resolved Hide resolved
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.

7 participants