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.

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: enable PX #363

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

feat: enable PX #363

wants to merge 8 commits into from

Conversation

sfroment
Copy link
Member

Signed-off-by: Sacha Froment sfroment42@gmail.com

@@ -216,6 +216,13 @@ async function main() {
render();
});

setInterval(() => {
peers = node.networkNode.getAllPeers();
discoveryPeers = node.networkNode.getGroupPeers("drp::discovery");
Copy link
Member

Choose a reason for hiding this comment

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

let's drop the listing of discoveryPeers on the examples (all)

Copy link
Member Author

Choose a reason for hiding this comment

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

are you sure ?

Copy link
Member

Choose a reason for hiding this comment

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

why not? is it useful information?

packages/network/src/node.ts Outdated Show resolved Hide resolved
IPColocationFactorWeight: 0,
appSpecificScore: (peerId: string) => {
if (_bootstrapPeerID.includes(peerId)) {
Copy link
Member

Choose a reason for hiding this comment

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

why are we creating a new variable and populating it instead of directly checking _bootstrapNodesList?

Copy link
Member Author

Choose a reason for hiding this comment

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

There the list is only the list of the peer id in the BT list not the full MA.
This is made in order not to recompute the same variable over and over

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (_bootstrapPeerID.includes(peerId)) {
if (_bootstrapNodesList.some((addr) => addr.includes(peerId))) {

how pre-populating is better than this? you don't recompute, it's only computed when you do the initialization (no?)

packages/network/src/node.ts Outdated Show resolved Hide resolved
},
}),
scoreThresholds: createPeerScoreThresholds({
gossipThreshold: -50,
Copy link
Member

Choose a reason for hiding this comment

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

comment inline why

packages/network/src/node.ts Show resolved Hide resolved
packages/network/src/node.ts Outdated Show resolved Hide resolved
packages/network/src/node.ts Outdated Show resolved Hide resolved
packages/network/src/node.ts Outdated Show resolved Hide resolved
packages/network/src/node.ts Show resolved Hide resolved
Signed-off-by: Sacha Froment <sfroment42@gmail.com>
Signed-off-by: Sacha Froment <sfroment42@gmail.com>
Signed-off-by: Sacha Froment <sfroment42@gmail.com>
Signed-off-by: Sacha Froment <sfroment42@gmail.com>
Signed-off-by: Sacha Froment <sfroment42@gmail.com>
Signed-off-by: Sacha Froment <sfroment42@gmail.com>
Signed-off-by: Sacha Froment <sfroment42@gmail.com>
Signed-off-by: Sacha Froment <sfroment42@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants