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.

Sign up for GitHub

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

Jump to bottom

Allow setting custom protocol in mplex #2332

Merged
merged 8 commits into from
Nov 23, 2021
Merged

Allow setting custom protocol in mplex #2332

merged 8 commits into from
Nov 23, 2021

Conversation

hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Nov 9, 2021 •
edited
Loading

in go-libp2p-muxer, it allows using custom protocol name. Adding a similar pub fn set_protocol to MplexConfig to connect to go nodes with custom protocol

// Muxer configures libp2p to use the given stream multiplexer (or stream
// multiplexer constructor).
//
// Name is the protocol name.
//
// The transport can be a constructed mux.Transport or a function taking any
// subset of this libp2p node's:
// * Peer ID
// * Host
// * Network
// * Peerstore
func Muxer(name string, tpt interface{}) Option {
	mtpt, err := config.MuxerConstructor(tpt)
	err = traceError(err, 1)
	return func(cfg *Config) error {
		if err != nil {
			return err
		}
		cfg.Muxers = append(cfg.Muxers, config.MsMuxC{MuxC: mtpt, ID: name})
		return nil
	}
}

Closes #2344

mxinden
mxinden reviewed Nov 19, 2021
View reviewed changes
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.

Could you include a changelog entry and bump the version?

https://github.com/libp2p/rust-libp2p/blob/master/docs/release.md

muxers/mplex/src/config.rs Outdated
Comment on lines 92 to 95
/// Gets protocol
pub fn protocol(&self) -> &[u8] {
self.protocol
}
Copy link
Member

Choose a reason for hiding this comment

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

None of the other fields have a getter method. Why is this one needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it's useful for debugging/logging purposes to be able to retrieve the protocol name when it can be customized

muxers/mplex/src/config.rs Outdated Show resolved Hide resolved
muxers/mplex/src/config.rs Outdated Show resolved Hide resolved
muxers/mplex/src/config.rs Outdated Show resolved Hide resolved
muxers/mplex/src/config.rs Outdated Show resolved Hide resolved
@hanabi1224
Copy link
Contributor Author

hanabi1224 commented Nov 20, 2021 •
edited
Loading

@mxinden Thanks for your review, I've updated the PR, please take another look.

@hanabi1224 hanabi1224 requested a review from mxinden November 20, 2021 05:10
mxinden
mxinden reviewed Nov 23, 2021
View reviewed changes
muxers/mplex/src/config.rs Outdated
Comment on lines 92 to 95
/// Gets the protocol anme
pub fn protocol_name(&self) -> &[u8] {
self.protocol_name
}
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the bringing this up once more, still, for the sake of consistency, I don't think we should introduce a getter for a single field only.

In case you would like to introduce getters for all fields, I think that should be a separate pull request.

Copy link
Contributor Author

@hanabi1224 hanabi1224 Nov 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Removed as suggested.

Although I don't really agree with the point of maintaining consistency. I think it makes sense to have immutable getters only for a subset of the fields, sharing the same rationale behind having different access modifiers for different fields in many languages including rust, but I do agree this might belong to a separate PR.

Hope this addresses ur concern in this PR, thx!

muxers/mplex/Cargo.toml Outdated
@@ -2,7 +2,7 @@
name = "libp2p-mplex"
edition = "2018"
description = "Mplex multiplexing protocol for libp2p"
version = "0.30.0"
version = "0.31.0"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
version = "0.31.0"
version = "0.30.1"

Is this a breaking change? If not, why not release it as a patch release?

muxers/mplex/CHANGELOG.md Outdated
@@ -1,3 +1,8 @@
# unreleased
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# unreleased
# 0.30.1 [unreleased]

@hanabi1224
Copy link
Contributor Author

@mxinden Comments resolved, plz take another look thx!

@hanabi1224 hanabi1224 requested a review from mxinden November 23, 2021 13:41
mxinden
mxinden approved these changes Nov 23, 2021
View reviewed changes
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.

Thanks @hanabi1224.

muxers/mplex/CHANGELOG.md Outdated Show resolved Hide resolved
@mxinden mxinden merged commit 243f587 into libp2p:master Nov 23, 2021
@hanabi1224 hanabi1224 deleted the mplex-custom-protocol branch November 23, 2021 19:28
@hanabi1224
Copy link
Contributor Author

Thanks!

canewsin added a commit to decentnetwork/rust-libp2p that referenced this pull request Nov 25, 2021
@canewsin @hanabi1224
@mxinden @dependabot
latest (#3)
559ea68 
* muxers/mplex: Allow setting custom protocol name (libp2p#2332)

Co-authored-by: Max Inden <[email protected]>

* build(deps): Update open-metrics-client requirement from 0.12.0 to 0.13.0 (libp2p#2355)

* build(deps): Update open-metrics-client requirement

Updates the requirements on [open-metrics-client](https://github.com/mxinden/rust-open-metrics-client) to permit the latest version.
- [Release notes](https://github.com/mxinden/rust-open-metrics-client/releases)
- [Changelog](https://github.com/mxinden/rust-open-metrics-client/blob/master/CHANGELOG.md)
- [Commits](prometheus/client_rust@v0.12.0...v0.13.0)

---
updated-dependencies:
- dependency-name: open-metrics-client
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Max Inden <[email protected]>

Co-authored-by: hanabi1224 <[email protected]>
Co-authored-by: Max Inden <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
vnermolaev pushed a commit to vnermolaev/rust-libp2p that referenced this pull request Nov 30, 2021
@hanabi1224 @mxinden
muxers/mplex: Allow setting custom protocol name (libp2p#2332)
eb46b71 
Co-authored-by: Max Inden <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Allow setting custom protocol in mplex
2 participants
@hanabi1224 @mxinden