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

Add support for protocol/agent version #64

Merged
merged 3 commits into from
Mar 21, 2024

Conversation

altonen
Copy link
Contributor

@altonen altonen commented Mar 20, 2024

Add ability to specify IPFS Identify protocol version and user agent. The implementation follows rust-libp2p, making protocol version mandatory and user agent is optional, which defaults to litep2p/1.0.0.

This needs a Polkadot SDK "companion" because it changes the public API. I can make the fix if I have push rights to the PR. The changes are pretty trivial though, just adding two new parameters to IdentifyConfig::new().

@dmitry-markin
Copy link
Collaborator

dmitry-markin commented Mar 21, 2024

@alvicsam are the environments for test runs in every PR cached? The tests now fail due to missing nightly toolchain and missing protoc (protobuf compiler). I wonder if we somehow pulled these dependencies working with #56.

Copy link
Collaborator

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

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

Thank you!

Do you need this PR merged asap? Otherwise I'd wait until we prepare the "companion" before merging this to not break polkadot-sdk building.

tests/conformance/rust/identify.rs Show resolved Hide resolved
tests/conformance/rust/identify.rs Outdated Show resolved Hide resolved
@alvicsam
Copy link
Contributor

@alvicsam are the environments for test runs in every PR cached? The tests now fail due to missing nightly toolchain and missing protoc (protobuf compiler). I wonder if we somehow pulled these dependencies working with #56.

It seems that repo variables don't work for PR that were made from forks. I will add a quick workaround and investigate it later.

dmitry-markin pushed a commit that referenced this pull request Mar 21, 2024
PR adds additional step to run all jobs from a container. The step is
needed because when CI runs from forks repo variables are not
accessible.

cc #64 (comment)
cc paritytech/ci_cd#964
@dmitry-markin
Copy link
Collaborator

dmitry-markin commented Mar 21, 2024

@alvicsam Do you know how to add "Update branch" button to merge master into the PR, like it's done in polkadot-sdk?

EDIT: may be it's hidden because there are no conflicts with master...

@alvicsam
Copy link
Contributor

@dmitry-markin I activated the button in the repo settings

@altonen
Copy link
Contributor Author

altonen commented Mar 21, 2024

Thank you!

Do you need this PR merged asap? Otherwise I'd wait until we prepare the "companion" before merging this to not break polkadot-sdk building.

You can use the integration PR as the "companion" since it also points to master. Your call though, I'm not in a rush to get this merged

Copy link
Collaborator

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

Looks clean to me! 👍

@dmitry-markin dmitry-markin merged commit fd0bdfb into paritytech:master Mar 21, 2024
8 checks passed
@dmitry-markin
Copy link
Collaborator

Merging this, and we'll update the polkadot-sdk integration PR later.

Thanks again @altonen !

@altonen
Copy link
Contributor Author

altonen commented Mar 21, 2024

Thank you @dmitry-markin and @lexnv for accepting!

dmitry-markin added a commit to paritytech/polkadot-sdk that referenced this pull request Mar 22, 2024
Upgrade litep2p identify in substrate to match
paritytech/litep2p#64.

Target branch is #2944.
@lexnv lexnv mentioned this pull request Apr 5, 2024
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.

4 participants