-
Notifications
You must be signed in to change notification settings - Fork 746
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
Devp2p DPT Type Improvements #1029
Changes from all commits
8b589f0
1e6d1c9
8cf09f5
73be137
b3b0e78
9fe77d6
2b2c0c2
9cc2ec7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,56 +1,58 @@ | ||
import { EventEmitter } from 'events' | ||
import _KBucket = require('k-bucket') | ||
import { PeerInfo } from './dpt' | ||
|
||
const KBUCKET_SIZE = 16 | ||
const KBUCKET_CONCURRENCY = 3 | ||
|
||
export interface KObj { | ||
id?: string | ||
port?: string | ||
address?: string | ||
export interface CustomContact extends PeerInfo { | ||
id: Uint8Array | Buffer | ||
vectorClock: number | ||
} | ||
|
||
export class KBucket extends EventEmitter { | ||
_peers: Map<string, any> = new Map() | ||
_peers: Map<string, PeerInfo> = new Map() | ||
_kbucket: _KBucket | ||
constructor(id: string | Buffer) { | ||
constructor(localNodeId: Buffer) { | ||
super() | ||
|
||
this._kbucket = new _KBucket({ | ||
localNodeId: typeof id === 'string' ? Buffer.from(id) : id, | ||
this._kbucket = new _KBucket<CustomContact>({ | ||
localNodeId, | ||
numberOfNodesPerKBucket: KBUCKET_SIZE, | ||
numberOfNodesToPing: KBUCKET_CONCURRENCY, | ||
}) | ||
|
||
this._kbucket.on('added', (peer: any) => { | ||
this._kbucket.on('added', (peer: PeerInfo) => { | ||
KBucket.getKeys(peer).forEach((key) => this._peers.set(key, peer)) | ||
this.emit('added', peer) | ||
}) | ||
|
||
this._kbucket.on('removed', (peer: any) => { | ||
this._kbucket.on('removed', (peer: PeerInfo) => { | ||
KBucket.getKeys(peer).forEach((key) => this._peers.delete(key)) | ||
this.emit('removed', peer) | ||
}) | ||
|
||
this._kbucket.on('ping', (...args: any[]) => this.emit('ping', ...args)) | ||
this._kbucket.on('ping', (oldPeers: PeerInfo[], newPeer: PeerInfo) => { | ||
this.emit('ping', { oldPeers, newPeer }) | ||
}) | ||
} | ||
|
||
static getKeys(obj: Buffer | string | KObj): string[] { | ||
static getKeys(obj: Buffer | string | PeerInfo): string[] { | ||
if (Buffer.isBuffer(obj)) return [obj.toString('hex')] | ||
if (typeof obj === 'string') return [obj] | ||
|
||
const keys = [] | ||
if (Buffer.isBuffer(obj.id)) keys.push(obj.id.toString('hex')) | ||
if (obj.address && obj.port) keys.push(`${obj.address}:${obj.port}`) | ||
//if (obj.address && obj.port) keys.push(`${obj.address}:${obj.port}`) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this remain commented out? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, there is just no property Will also leave to not revoke the approval but rather merge here now. |
||
return keys | ||
} | ||
|
||
add(peer: any) { | ||
add(peer: PeerInfo) { | ||
const isExists = KBucket.getKeys(peer).some((key) => this._peers.has(key)) | ||
if (!isExists) this._kbucket.add(peer) | ||
if (!isExists) this._kbucket.add(peer as CustomContact) | ||
} | ||
|
||
get(obj: Buffer | string | KObj) { | ||
get(obj: Buffer | string | PeerInfo) { | ||
for (const key of KBucket.getKeys(obj)) { | ||
const peer = this._peers.get(key) | ||
if (peer !== undefined) return peer | ||
|
@@ -59,16 +61,16 @@ export class KBucket extends EventEmitter { | |
return null | ||
} | ||
|
||
getAll(): Array<any> { | ||
getAll(): Array<PeerInfo> { | ||
return this._kbucket.toArray() | ||
} | ||
|
||
closest(id: string): any { | ||
closest(id: string): PeerInfo[] { | ||
return this._kbucket.closest(Buffer.from(id), KBUCKET_SIZE) | ||
} | ||
|
||
remove(obj: Buffer | string | KObj) { | ||
remove(obj: Buffer | string | PeerInfo) { | ||
const peer = this.get(obj) | ||
if (peer !== null) this._kbucket.remove(peer.id) | ||
if (peer !== null) this._kbucket.remove((peer as CustomContact).id) | ||
} | ||
} |
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.
codecov is warning that this line isn't covered, it's not so important, but could add a quick test forcing a fast timeout to ensure the error is caught and propagated properly
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 will leave this for now but generally that's a good idea - also left a comment on the codecov/patch discussion in the chat. Will likely be good if we get to some work mode where we take coverage more serious again (I at least started in #1028 to add some substantial tests and had a look at codecov/patch, took me quite some time but I think I finally had some mental breakthrough in my
testdouble
understanding 😄 ).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've directly taken the occasion and made the
codecov/project
check mandatory again, together with the other library'stest-*
checks (e.g.test-trie
), think we still had some gap here due to uncertainties in the CI setup for some time (these "run everything or run selected" discussions). And we can of course later always adopt again if some CI setup change comes up.