Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

feat: remove dht.getPublicKey #258

Closed
wants to merge 1 commit into from

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Dec 1, 2021

The dht.getPublicKey method is a specialised dht.get so you should just call that with the right key value.

This lets us remove the libp2p-crypto dependency as we were using it to unmarshal the returned key - if you are interested in a node's public key you're probably about to do something with it in which case you probably have a dep on libp2p-crypto so no need for this module to have one.

BREAKING CHANGE: dht.getPublicKey has been removed

src/dual-kad-dht.js Outdated Show resolved Hide resolved
@achingbrain achingbrain requested review from jacobheun, vasco-santos and hugomrdias and removed request for jacobheun December 1, 2021 12:21
@hugomrdias
Copy link
Member

Im being picky here but that stablelib doesnt do the node/browser swap includes both and if the bundler doesnt do treeshaking it includes even more stuff.

Perfect scenario would be libp2p-crypto/random or a new package with the https://github.com/hugomrdias/iso-random-stream/blob/master/src/random.browser.js (could be improved with the clever for from the stablelib impl) , https://github.com/hugomrdias/iso-random-stream/blob/master/src/random.js and the browser swap in package.json.

the rest LGTM

@achingbrain
Copy link
Member Author

achingbrain commented Dec 1, 2021

Im being picky here

That's fine, I've not got a strong opinion about which randomBytes implementation to use.

Perfect scenario would be libp2p-crypto/random

I disagree because this module only needs a random number generator, not a full crypto suite.

Yes, a clever bundler would make sure only what's used is bundled, but for example if we wanted to make a breaking change in libp2p-crypto/foo-bar, this module would be involved in the roll-out of that breaking change even though it doesn't use that code at all.

or a new package with the hugomrdias/iso-random-stream@master/src/random.browser.js (could be improved with the clever for from the stablelib impl) , hugomrdias/iso-random-stream@master/src/random.js and the browser swap in package.json.

Sounds preferable - would merge!

@achingbrain achingbrain force-pushed the feat/remove-libp2p-crypto-dependency branch from 9bc0d96 to 366256d Compare December 1, 2021 16:10
@achingbrain
Copy link
Member Author

@hugomrdias I've reverted to iso-random-stream but it needs hugomrdias/iso-random-stream#87 before the build will pass here.

The `dht.getPublicKey` method is a specialised `dht.get` so you
should just call that with the right key value.

This lets us remove the `libp2p-crypto` dependency as we were using
it to unmarshal the returned key - if you are interested in a
node's public key you're probably about to do something with it
in which case you probably have a dep on `libp2p-crypto` so no need
for this module to have one.

BREAKING CHANGE: `dht.getPublicKey` has been removed

chore: update iso random stream

chore: remove
@achingbrain achingbrain force-pushed the feat/remove-libp2p-crypto-dependency branch from a13ab55 to 9b466de Compare December 2, 2021 18:15
@achingbrain achingbrain changed the title feat: remove libp2p-crypto dependency feat: remove dht.getPublicKey Dec 2, 2021
@achingbrain
Copy link
Member Author

Superseded by #300

@achingbrain achingbrain deleted the feat/remove-libp2p-crypto-dependency branch March 24, 2022 12:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants