Skip to content
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.

Sign up for GitHub

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

Feat/peerstore impl #505

Merged
merged 6 commits into from
Jan 29, 2021
Merged

Feat/peerstore impl #505

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c506272
Added missing books
jm-clius Jan 15, 2021
48be0a7
Extract handlers from the encapsulating listener object.
jm-clius Jan 15, 2021
ff7d081
Improvement - move handlers to books
jm-clius Jan 19, 2021
926d340
Cleanup unnecessary newSeq initialisations
jm-clius Jan 19, 2021
12e0716
Genericise PeerStore
jm-clius Jan 26, 2021
218ca64
Removed metadataBook (for now)
jm-clius Jan 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Extract handlers from the encapsulating listener object.
  • Loading branch information
@jm-clius
jm-clius committed Jan 15, 2021
commit 48be0a7d25b4217e46f23c54adac1d44bf949a80
55 changes: 30 additions & 25 deletions libp2p/peerstore.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,14 @@ import
./multiaddress

type
##############################
# Listener and handler types #
##############################
#################
# Handler types #
#################

AddrChangeHandler* = proc(peerId: PeerID, multiaddrs: HashSet[MultiAddress])
ProtoChangeHandler* = proc(peerId: PeerID, protos: HashSet[string])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is FooChangeHandler the common name here? Nitpick, but something like FooUpdateHandler might be a tiny bit more descriptive, "Change" to me suggests something could be breaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. change was selected to stay as close as possible to similar events and naming in the js-implementation, but I am happy to update naming to update :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the naming convention makes sense here.

KeyChangeHandler* = proc(peerId: PeerID, publicKey: PublicKey)
MetadataChangeHandler* = proc(peerId: PeerID, metadata: Table[string, seq[byte]])

EventListener* = object
# Listener object with client-defined handlers for peer store events
addrChange*: AddrChangeHandler
protoChange*: ProtoChangeHandler
keyChange*: KeyChangeHandler
metadataChange*: MetadataChangeHandler

#########
# Books #
Expand Down Expand Up @@ -60,7 +53,10 @@ type
protoBook*: ProtoBook
keyBook*: KeyBook
metadataBook*: MetadataBook
listeners: seq[EventListener]
addrChangeHandlers*: seq[AddrChangeHandler]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, wouldn't it make more sense to move the change handlers into the specific *Book types? For example, I'm not sure what the functionality of KeyBook.keyChange is and how it relates to keyChangeHandlers? Wouldn't it make more sense to move keyChangeHandlers under KeyBook and get rid of keyChange, or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. :) Improvement in ff7d081

The idea was to create some level of indirection between the multiple public handlers that different clients can register on the Peer Store and the single, private handler that the peer store itself registers per *Book when it initialises those books. Each book then only notifies its encompassing Peer Store of changes and the Peer Store in turn notifies its multiple registered clients (perhaps enforcing optional policy). Currently this serves no more than a conceptual purpose, so it will be cleaner to just move the handlers to the individual books. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure if the "proxy" handler makes sense there - but I understand the idea behind it now.

My perspective on the PeerStore is a wrapper that exposes specific register*Handler methods to each underlying *Book. I think it's cleaner and simpler to reason about, but I'm willing to reconsider it when we might require this functionality.

protoChangeHandlers*: seq[ProtoChangeHandler]
keyChangeHandlers*: seq[KeyChangeHandler]
metadataChangeHandlers*: seq[MetadataChangeHandler]

StoredInfo* = object
# Collates stored info about a peer
Expand All @@ -87,27 +83,30 @@ proc init(T: type MetadataBook, metadataChange: MetadataChangeHandler): Metadata
metadataChange: metadataChange)

proc init*(p: PeerStore) =
p.listeners = newSeq[EventListener]()
p.addrChangeHandlers = newSeq[AddrChangeHandler]()
p.protoChangeHandlers = newSeq[ProtoChangeHandler]()
p.keyChangeHandlers = newSeq[KeyChangeHandler]()
p.metadataChangeHandlers = newSeq[MetadataChangeHandler]()

proc addrChange(peerId: PeerID, multiaddrs: HashSet[MultiAddress]) =
# Notify all listeners of change in multiaddr
for listener in p.listeners:
listener.addrChange(peerId, multiaddrs)
for handler in p.addrChangeHandlers:
handler(peerId, multiaddrs)

proc protoChange(peerId: PeerID, protos: HashSet[string]) =
# Notify all listeners of change in proto
for listener in p.listeners:
listener.protoChange(peerId, protos)
for handler in p.protoChangeHandlers:
handler(peerId, protos)

proc keyChange(peerId: PeerID, publicKey: PublicKey) =
# Notify all listeners of change in public key
for listener in p.listeners:
listener.keyChange(peerId, publicKey)
for handler in p.keyChangeHandlers:
handler(peerId, publicKey)

proc metadataChange(peerId: PeerID, metadata: Table[string, seq[byte]]) =
# Notify all listeners of change in public key
for listener in p.listeners:
listener.metadataChange(peerId, metadata)
for handler in p.metadataChangeHandlers:
handler(peerId, metadata)

p.addressBook = AddressBook.init(addrChange)
p.protoBook = ProtoBook.init(protoChange)
Expand Down Expand Up @@ -299,11 +298,17 @@ proc deleteValue*(metadataBook: var MetadataBook,
# Peer Store API #
##################

proc addListener*(peerStore: PeerStore,
listener: EventListener) =
## Register event listener to notify clients of changes in the peer store

peerStore.listeners.add(listener)
proc addHandlers*(peerStore: PeerStore,
addrChangeHandler: AddrChangeHandler,
protoChangeHandler: ProtoChangeHandler,
keyChangeHandler: KeyChangeHandler,
metadataChangeHandler: MetadataChangeHandler) =
## Register event handlers to notify clients of changes in the peer store

peerStore.addrChangeHandlers.add(addrChangeHandler)
peerStore.protoChangeHandlers.add(protoChangeHandler)
peerStore.keyChangeHandlers.add(keyChangeHandler)
peerStore.metadataChangeHandlers.add(metadataChangeHandler)

proc delete*(peerStore: PeerStore,
peerId: PeerID): bool =
Expand Down
10 changes: 4 additions & 6 deletions tests/testpeerstore.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,11 @@ suite "PeerStore":

proc metadataChange(peerId: PeerID, metadata: Table[string, seq[byte]]) =
metadataChanged = true

let listener = EventListener(addrChange: addrChange,
protoChange: protoChange,
keyChange: keyChange,
metadataChange: metadataChange)

peerStore.addListener(listener)
peerStore.addHandlers(addrChangeHandler = addrChange,
protoChangeHandler = protoChange,
keyChangeHandler = keyChange,
metadataChangeHandler = metadataChange)

# Test listener triggered on adding multiaddr
peerStore.addressBook.add(peerId1, multiaddr1)
Expand Down