-
Notifications
You must be signed in to change notification settings - Fork 446
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.
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
chore: deprecate old peer store api #598
Conversation
904226b
to
98004ad
Compare
61563bb
to
d2b9960
Compare
c9b3248
to
fb66739
Compare
af6454f
to
a2098bd
Compare
This should be ready to review @jacobheun ! FYI. the interop tests are failing here, but I can run them locally with any problems. Not exactly sure yet about the issue, but I think you can start reviewing this, as well as the dht PR. They depend on from one another, so I would say that we should probably make a beta release in the dht |
BREAKING CHANGE: the peer-store api changed. Check the API docs for the new specification.
56b70f9
to
060d69a
Compare
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.
A few minor things, otherwise this is looking good.
src/peer-store/address-book.js
Outdated
@@ -183,6 +185,12 @@ class AddressBook extends Book { | |||
return multiaddrInfos | |||
} | |||
|
|||
_setPeerId (peerId) { |
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.
Add this to the base Book class?
src/peer-store/proto-book.js
Outdated
@@ -132,6 +134,12 @@ class ProtoBook extends Book { | |||
|
|||
return this | |||
} | |||
|
|||
_setPeerId (peerId) { |
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.
Same as above, add to Book class.
@@ -96,7 +95,7 @@ describe('protoBook', () => { | |||
const supportedProtocols = ['protocol1', 'protocol2'] | |||
|
|||
let changeCounter = 0 | |||
ee.on('change:protocols', () => { | |||
peerStore.on('change:protocols', () => { |
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.
I think we should add the events to the API docs at https://github.com/libp2p/js-libp2p/blob/0.28.x/doc/API.md#events. We could split that into subsections (libp2p
, libp2p.peerStore
) to link to from the TOC. WDYT?
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.
Yes, that makes sense. Good call
d805aba
to
7248b4c
Compare
Co-Authored-By: Jacob Heun <[email protected]>
7248b4c
to
655dd74
Compare
Addressed you review, the only tests failing are related to the delegated routing issue that is being fixed. I also made a pre release for the dht, we just need to figure out how we should merge this, with the interop dependency that needs the daemon updated. |
doc/API.md
Outdated
@@ -41,6 +41,8 @@ | |||
* [`metrics.forPeer`](#metricsforpeer) | |||
* [`metrics.forProtocol`](#metricsforprotocol) | |||
* [Events](#events) | |||
* [`libp2p`](#eventslibp2p) | |||
* [`libp2p.peerStore`](#eventslibp2ppeerStore) |
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.
Both of these anchor links won't work, they shouldn't be prefaced with events
We could leave it as a branch for now. The other option could be to cherry pick out the daemon specification updates in libp2p/interop#32. That would enable us to do a pre release of the daemon and just install it and use it in the interop tests here on CI. |
Co-Authored-By: Jacob Heun <[email protected]>
712a69b
to
dbc6210
Compare
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
* chore: deprecate old peer-store api BREAKING CHANGE: the peer-store api changed. Check the API docs for the new specification. * chore: apply suggestions from code review Co-Authored-By: Jacob Heun <[email protected]> * chore: apply suggestions from code review Co-Authored-By: Jacob Heun <[email protected]> Co-authored-by: Jacob Heun <[email protected]>
* chore: deprecate old peer-store api BREAKING CHANGE: the peer-store api changed. Check the API docs for the new specification. * chore: apply suggestions from code review Co-Authored-By: Jacob Heun <[email protected]> * chore: apply suggestions from code review Co-Authored-By: Jacob Heun <[email protected]> Co-authored-by: Jacob Heun <[email protected]>
* chore: deprecate old peer-store api BREAKING CHANGE: the peer-store api changed. Check the API docs for the new specification. * chore: apply suggestions from code review Co-Authored-By: Jacob Heun <[email protected]> * chore: apply suggestions from code review Co-Authored-By: Jacob Heun <[email protected]> Co-authored-by: Jacob Heun <[email protected]>
This PR is a follow up of #590 on the context of the PeerStore improvements described on #582
It removes the older peerStore API and adds a temporary record of peer id's. This is needed because the dht needs the peerStore to keep track of peers' public key. Once we have the key book, it will be removed.
Needs:
Unblocks:
BREAKING CHANGE: the peer-store api changed. Check the API docs for the new specification.