chore: use new peer store api #179
Conversation
| @@ -76,14 +76,14 @@ | |||
| "delay": "^4.3.0", | |||
| "dirty-chai": "^2.0.1", | |||
| "it-pair": "^1.0.0", | |||
| "libp2p": "libp2p/js-libp2p#chore/deprecate-old-peer-store-api", | |||
There was a problem hiding this comment.
@jacobheun I am not sure if mocking the peerStore is the best thing to do here, or depend on libp2p as a dev dependency again. What do you think? It could be worth having a mocks repo for this kind of stuff, but we would need to make sure it was properly updated with new changes
There was a problem hiding this comment.
@vasco-santos something I think could be a better setup is to have libp2p as a dev dependency of the modules, and then have the libp2p test suite just try to run these tests as part of CI. I know we'd need to overcome some hurdles of what happens during breaking change releases, but this might be an opportunity to trial that flow with the 0.28 release and maybe just do the DHT to start with. If we're able to make that a smooth process, we could do that for the other modules.
This gets us a step in that direction, so I am good with having it as a dev dep.
| return dht.peerStore.get(p) | ||
| } | ||
|
|
||
| return dht.peerStore.put(new PeerInfo(p)) |
There was a problem hiding this comment.
Already known providers, no need to add to the peerStore. Moreover, the peerInfo is just created here, so there is nothing relevant to add
| @@ -244,7 +244,6 @@ class WorkerQueue { | |||
| if (this.dht._isSelf(closer.id)) { | |||
| return | |||
| } | |||
| closer = this.dht.peerStore.put(closer) | |||
There was a problem hiding this comment.
No new information of the peer at this point
There was a problem hiding this comment.
Also it's unnecessary as the emit below will cause libp2p to update the peer store
vasco-santos
force-pushed
the
chore/use-new-peer-store-api
branch
2 times, most recently
from
8c59462
to
5656d6b
Compare
vasco-santos
force-pushed
the
chore/use-new-peer-store-api
branch
from
5656d6b
to
83f47c9
Compare
vasco-santos
force-pushed
the
chore/use-new-peer-store-api
branch
from
83f47c9
to
6946208
Compare
BREAKING CHANGE: uses new peer-store api
vasco-santos
force-pushed
the
chore/use-new-peer-store-api
branch
from
6946208
to
e5a0082
Compare
| @@ -76,14 +76,14 @@ | |||
| "delay": "^4.3.0", | |||
| "dirty-chai": "^2.0.1", | |||
| "it-pair": "^1.0.0", | |||
| "libp2p": "libp2p/js-libp2p#chore/deprecate-old-peer-store-api", | |||
There was a problem hiding this comment.
@vasco-santos something I think could be a better setup is to have libp2p as a dev dependency of the modules, and then have the libp2p test suite just try to run these tests as part of CI. I know we'd need to overcome some hurdles of what happens during breaking change releases, but this might be an opportunity to trial that flow with the 0.28 release and maybe just do the DHT to start with. If we're able to make that a smooth process, we could do that for the other modules.
This gets us a step in that direction, so I am good with having it as a dev dep.
| @@ -169,7 +173,7 @@ module.exports = (dht) => { | |||
| result.paths.forEach((result) => { | |||
| if (result.success) { | |||
| success = true | |||
| dht.peerStore.put(result.peer) | |||
| dht.peerStore.addressBook.add(result.peer.id, result.peer.multiaddrs.toArray()) | |||
There was a problem hiding this comment.
I like that this is now a discrete action. It's much clearer what's happening than a put imo.
| @@ -244,7 +244,6 @@ class WorkerQueue { | |||
| if (this.dht._isSelf(closer.id)) { | |||
| return | |||
| } | |||
| closer = this.dht.peerStore.put(closer) | |||
There was a problem hiding this comment.
Also it's unnecessary as the emit below will cause libp2p to update the peer store
In the context of the PeerStore improvements specified on libp2p/js-libp2p#582 and as a follow up of libp2p/js-libp2p#590, this PR switches
libp2p-kad-dhtusage ofpeer-storeto its new API.Moreover, there were some places where data was being added to the
peerStorewith any benefit. In some cases the data was already there and in other cases there was nothing new to add at that point.BREAKING CHANGE: uses new peer-store api
Needs:
Unblocks: