-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat!: set peer-exchange with default bootstrap #1469
Changes from 69 commits
29cb6a8
3646fdc
8d48380
573fb8d
4894091
a5c1372
6c1ec84
a0f0668
f376334
f7f383b
6807888
f956f7f
b43fe35
d712e8a
8bf969a
3204267
645d7c1
9d70d55
12cec4a
773d1ef
f345496
f2c2fd0
efef7a2
6a17cc3
befbd37
aa87190
83d5103
63796db
9583bda
6082ffb
c63d146
6fdcf61
916af18
5dff864
f75b2d9
7d14956
1c93c12
016965b
04b9bb5
1f81ae1
dce7b3b
a376bdd
ec3bcc8
c697c15
1b0e1fe
c7fca3e
191355b
6820ebe
e968d4d
401265b
1b91278
75ee463
56d8b84
dcb4fbf
2cd8fa0
ad518af
f24e8c0
7354b09
c1d2e1a
919f10b
e5f1331
6a1a4ad
a6cad7a
0f629cb
9302a86
50df227
053c9da
5aa4fcb
afbcaae
78114c7
d44670d
9439bb3
1e2a9d2
7e49625
c90f88b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,143 @@ | ||
import { Peer } from "@libp2p/interface/peer-store"; | ||
import type { Tag } from "@libp2p/interface/peer-store"; | ||
import { createSecp256k1PeerId } from "@libp2p/peer-id-factory"; | ||
import { Tags } from "@waku/interfaces"; | ||
import { filterPeers } from "@waku/utils"; | ||
import { expect } from "chai"; | ||
|
||
describe("getPeers function", function () { | ||
it("should return all peers when numPeers is 0", async function () { | ||
const peer1 = await createSecp256k1PeerId(); | ||
const peer2 = await createSecp256k1PeerId(); | ||
const peer3 = await createSecp256k1PeerId(); | ||
|
||
const mockPeers = [ | ||
{ | ||
id: peer1, | ||
tags: new Map<string, Tag>([[Tags.BOOTSTRAP, { value: 100 }]]) | ||
}, | ||
{ | ||
id: peer2, | ||
tags: new Map<string, Tag>([[Tags.BOOTSTRAP, { value: 100 }]]) | ||
}, | ||
{ | ||
id: peer3, | ||
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. I'd include non-boostrap peers in this test to ensure they don't get filtered out. |
||
tags: new Map<string, Tag>([[Tags.BOOTSTRAP, { value: 100 }]]) | ||
} | ||
] as unknown as Peer[]; | ||
|
||
const result = await filterPeers(mockPeers, 0, 10); | ||
expect(result.length).to.deep.equal(mockPeers.length); | ||
}); | ||
|
||
it("should return all non-bootstrap peers and no bootstrap peer when numPeers is 0 and maxBootstrapPeers is 0", async function () { | ||
const peer1 = await createSecp256k1PeerId(); | ||
const peer2 = await createSecp256k1PeerId(); | ||
const peer3 = await createSecp256k1PeerId(); | ||
const peer4 = await createSecp256k1PeerId(); | ||
|
||
const mockPeers = [ | ||
{ | ||
id: peer1, | ||
tags: new Map<string, Tag>([[Tags.BOOTSTRAP, { value: 100 }]]) | ||
}, | ||
{ | ||
id: peer2, | ||
tags: new Map<string, Tag>([[Tags.BOOTSTRAP, { value: 100 }]]) | ||
}, | ||
{ | ||
id: peer3, | ||
tags: new Map<string, Tag>([[Tags.PEER_EXCHANGE, { value: 100 }]]) | ||
}, | ||
{ | ||
id: peer4, | ||
tags: new Map<string, Tag>([[Tags.PEER_EXCHANGE, { value: 100 }]]) | ||
} | ||
] as unknown as Peer[]; | ||
|
||
const result = await filterPeers(mockPeers, 0, 0); | ||
|
||
// result should have no bootstrap peers, and a total of 2 peers | ||
expect(result.length).to.equal(2); | ||
expect( | ||
result.filter((peer: Peer) => peer.tags.has(Tags.BOOTSTRAP)).length | ||
).to.equal(0); | ||
}); | ||
|
||
it("should return one bootstrap peer, and all non-boostrap peers, when numPeers is 0 & maxBootstrap is 1", async function () { | ||
const peer1 = await createSecp256k1PeerId(); | ||
const peer2 = await createSecp256k1PeerId(); | ||
const peer3 = await createSecp256k1PeerId(); | ||
const peer4 = await createSecp256k1PeerId(); | ||
const peer5 = await createSecp256k1PeerId(); | ||
|
||
const mockPeers = [ | ||
{ | ||
id: peer1, | ||
tags: new Map<string, Tag>([[Tags.BOOTSTRAP, { value: 100 }]]) | ||
}, | ||
{ | ||
id: peer2, | ||
tags: new Map<string, Tag>([[Tags.BOOTSTRAP, { value: 100 }]]) | ||
}, | ||
{ | ||
id: peer3, | ||
tags: new Map<string, Tag>([[Tags.PEER_EXCHANGE, { value: 100 }]]) | ||
}, | ||
{ | ||
id: peer4, | ||
tags: new Map<string, Tag>([[Tags.PEER_EXCHANGE, { value: 100 }]]) | ||
}, | ||
{ | ||
id: peer5, | ||
tags: new Map<string, Tag>([[Tags.PEER_EXCHANGE, { value: 100 }]]) | ||
} | ||
] as unknown as Peer[]; | ||
|
||
const result = await filterPeers(mockPeers, 0, 1); | ||
|
||
// result should have 1 bootstrap peers, and a total of 4 peers | ||
expect(result.length).to.equal(4); | ||
expect( | ||
result.filter((peer: Peer) => peer.tags.has(Tags.BOOTSTRAP)).length | ||
).to.equal(1); | ||
}); | ||
|
||
it("should return only bootstrap peers up to maxBootstrapPeers", async function () { | ||
const peer1 = await createSecp256k1PeerId(); | ||
const peer2 = await createSecp256k1PeerId(); | ||
const peer3 = await createSecp256k1PeerId(); | ||
const peer4 = await createSecp256k1PeerId(); | ||
const peer5 = await createSecp256k1PeerId(); | ||
|
||
const mockPeers = [ | ||
{ | ||
id: peer1, | ||
tags: new Map<string, Tag>([[Tags.BOOTSTRAP, { value: 100 }]]) | ||
}, | ||
{ | ||
id: peer2, | ||
tags: new Map<string, Tag>([[Tags.BOOTSTRAP, { value: 100 }]]) | ||
}, | ||
{ | ||
id: peer3, | ||
tags: new Map<string, Tag>([[Tags.BOOTSTRAP, { value: 100 }]]) | ||
}, | ||
{ | ||
id: peer4, | ||
tags: new Map<string, Tag>([[Tags.PEER_EXCHANGE, { value: 100 }]]) | ||
}, | ||
{ | ||
id: peer5, | ||
tags: new Map<string, Tag>([[Tags.PEER_EXCHANGE, { value: 100 }]]) | ||
} | ||
] as unknown as Peer[]; | ||
|
||
const result = await filterPeers(mockPeers, 5, 2); | ||
|
||
// check that result has at least 2 bootstrap peers and no more than 5 peers | ||
expect(result.length).to.be.at.least(2); | ||
expect(result.length).to.be.at.most(5); | ||
expect(result.filter((peer: Peer) => peer.tags.has(Tags.BOOTSTRAP)).length); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -1,5 +1,4 @@ | ||||
import { Stream } from "@libp2p/interface/connection"; | ||||
import type { PeerId } from "@libp2p/interface/peer-id"; | ||||
import type { Peer } from "@libp2p/interface/peer-store"; | ||||
import type { IncomingStreamData } from "@libp2p/interface-internal/registrar"; | ||||
import type { | ||||
|
@@ -14,7 +13,6 @@ import type { | |||
Libp2p, | ||||
PeerIdStr, | ||||
ProtocolCreateOptions, | ||||
ProtocolOptions, | ||||
PubSubTopic, | ||||
Unsubscribe | ||||
} from "@waku/interfaces"; | ||||
|
@@ -228,6 +226,7 @@ class Subscription { | |||
class Filter extends BaseProtocol implements IReceiver { | ||||
private readonly options: ProtocolCreateOptions; | ||||
private activeSubscriptions = new Map<string, Subscription>(); | ||||
private readonly NUM_PEERS_PROTOCOL = 1; | ||||
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. I see that we always define 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. Maybe we can place in 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. this is a design decision to keep 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. ref: #1469 (comment) 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. no problems with codecs, we have other places with constants defined on top
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. i agree with you and incline towards moving constants to a dedicated package 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. oh, I don't really think it should be a separate package, to me it seems good enough that we have for local constans to classes I think current structure is good that we have import { Foo } from "./foo";
...
const CONST_1 = 1;
const CONST_2 = 2;
...
export class A {}
... 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. gotcha. tracked here: #1535 |
||||
|
||||
private getActiveSubscription( | ||||
pubSubTopic: PubSubTopic, | ||||
|
@@ -257,14 +256,16 @@ class Filter extends BaseProtocol implements IReceiver { | |||
this.options = options ?? {}; | ||||
} | ||||
|
||||
async createSubscription( | ||||
pubSubTopic?: string, | ||||
peerId?: PeerId | ||||
): Promise<Subscription> { | ||||
async createSubscription(pubSubTopic?: string): Promise<Subscription> { | ||||
const _pubSubTopic = | ||||
pubSubTopic ?? this.options.pubSubTopic ?? DefaultPubSubTopic; | ||||
|
||||
const peer = await this.getPeer(peerId); | ||||
const peer = ( | ||||
await this.getPeers({ | ||||
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. uses the new API but only uses one peer for filter |
||||
maxBootstrapPeers: 1, | ||||
numPeers: this.NUM_PEERS_PROTOCOL | ||||
}) | ||||
)[0]; | ||||
|
||||
const subscription = | ||||
this.getActiveSubscription(_pubSubTopic, peer.id.toString()) ?? | ||||
|
@@ -278,10 +279,9 @@ class Filter extends BaseProtocol implements IReceiver { | |||
} | ||||
|
||||
public toSubscriptionIterator<T extends IDecodedMessage>( | ||||
decoders: IDecoder<T> | IDecoder<T>[], | ||||
opts?: ProtocolOptions | undefined | ||||
decoders: IDecoder<T> | IDecoder<T>[] | ||||
): Promise<IAsyncIterator<T>> { | ||||
return toAsyncIterator(this, decoders, opts); | ||||
return toAsyncIterator(this, decoders); | ||||
} | ||||
|
||||
/** | ||||
|
@@ -301,10 +301,9 @@ class Filter extends BaseProtocol implements IReceiver { | |||
*/ | ||||
async subscribe<T extends IDecodedMessage>( | ||||
decoders: IDecoder<T> | IDecoder<T>[], | ||||
callback: Callback<T>, | ||||
opts?: ProtocolOptions | ||||
callback: Callback<T> | ||||
): Promise<Unsubscribe> { | ||||
const subscription = await this.createSubscription(undefined, opts?.peerId); | ||||
const subscription = await this.createSubscription(); | ||||
|
||||
await subscription.subscribe(decoders, callback); | ||||
|
||||
|
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.
This test file only tests
filterPeers
which is in a different package.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.
renamed this to be a
filterPeers
spec test instead