feat: support custom compat protocol name#42
feat: support custom compat protocol name#42hanabi1224 wants to merge 3 commits intoipfs-rust:masterfrom
Conversation
hanabi1224
force-pushed
the
custom-compat-protocol
branch
from
7e0bdf4 to
adef753
Compare
rkuhn
left a comment
rkuhn
left a comment
There was a problem hiding this comment.
Sorry for the delay, I had to mute github notifications for a while.
This change looks good to me, just two points to discuss.
src/behaviour.rs
Outdated
| use libp2p::core::{connection::ConnectionId, Multiaddr, PeerId}; | ||
| use libp2p::swarm::derive_prelude::{ConnectionClosed, DialFailure, FromSwarm, ListenFailure}; | ||
| #[cfg(feature = "compat")] | ||
| use libp2p::swarm::SubstreamProtocol; |
There was a problem hiding this comment.
please fold this into the following import
|
|
||
| fn protocol_info(&self) -> Self::InfoIter { | ||
| iter::once(b"/ipfs/bitswap/1.2.0") | ||
| unimplemented!() |
There was a problem hiding this comment.
This smells bad: why not instead remove this impl?
There was a problem hiding this comment.
It requires more refactoring to remove UpgradeInfo impl for CompatMessage
There was a problem hiding this comment.
That refactoring is the required proof that this unimplemented!() won’t panic at runtime, though. In other words, I find it reasonable to demand that a production ready library does not contain unimplemented!().
There was a problem hiding this comment.
That makes sense! However, it's been too long since I made the PR, and I do need some extra time to pick up the context and made the change.
2 participants
Changes:
go-bitswapcompatible nodelibipldto make life easier when there're version conflicts in downstream crates. (e.g.libipld@0.15depends on a differentcidversion fromfvm_ipld_blockstore) and it's not yet possible to upgradefvm_ipld_blockstore, see chore: upgradecid,multihash,libipld-corecrates filecoin-project/ref-fvm#1380 (comment)Background:
go-bitswapallows customizing the protocol name with itsSettings.PrefixAPI, e.g. lotus uses"/chain"prefix,rust-bitswapshould be capable of matching the protocol name incompatmode. Please let me know your feedback, thanks!