-
Notifications
You must be signed in to change notification settings - Fork 60
Feature: adds custom multicodec protocol option #206
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. The code snippet in the description would be great to have in the readme as well.
This is necessary and useful for running an isolated DHT and I think it's a reasonable first step for running multiple DHTs, although we're likely losing some efficiencies with this approach. This is also inline with how the go-libp2p DHT is configured and I believe it's currently using this approach to run DHTs for lan/wan networks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good. Can you please add @jacobheun recommendation as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just some minor adjustments in the README left
README.md
Outdated
@@ -42,6 +42,29 @@ | |||
const KadDHT = require('libp2p-kad-dht') | |||
``` | |||
|
|||
### Usage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would feel like the regular usage of the DHT
### Usage | |
### Custom secondary DHT in libp2p |
@HexaField apologies, but we should do another change to be consistent with go-libp2p. In This may help with network upgrades where we might want to be checking both /myapp/kad/1.0.0 and /myapp/kad/2.0.0 without asking users to understand what's happening with the upgrade. So, my suggestion here would be to rename the protocol option to Other than this, I would probably simplify this to another option parameter in create, such as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! Just missing the below suggestion for the legacy type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR adds the ability to use unique multicodec protocols for the DHT, which enables multiple DHTs to operate separately in a single libp2p instance, as well as specifying they are unique from the IPFS ecosystem.
Includes a test to ensure peers with separate protocols do not find each other. I have also run external integration tests with a libp2p instance with no problems. I am open to adding more tests if there are more cases to test for.
The protocol to use can be specified by passing the
protocol
argument in the constructor argument, which defaults to what currently is in use:"/ipfs/kad/1.0.0"
The
multicodec
module export used in other js-libp2p packages still exports the same default value, so all external references to it should remain unaffected.Please note, this may not a complete solution, since each DHT instance will have it's own peer topology while using the same peerStore. My knowledge of libp2p is limited in this capacity, so more work may need to be done to enable full functionality with multiple topologies.
For those interested, the code necessary to add a custom secondary DHT to a libp2p instance is as follows: