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

fix: Update libp2p deps #419

Merged
merged 14 commits into from
Jan 19, 2024
Merged

fix: Update libp2p deps #419

merged 14 commits into from
Jan 19, 2024

Conversation

akim-bow
Copy link
Contributor

No description provided.

@akim-bow akim-bow requested a review from shamsartem January 18, 2024 13:16
Copy link

linear bot commented Jan 18, 2024

@akim-bow akim-bow added the e2e Run e2e workflow label Jan 18, 2024
@akim-bow akim-bow requested a review from folex January 18, 2024 20:16
Comment on lines 85 to 94
const DOCKER_NOX_DENY_CONDITION: DenyCondition = (ma) => {
const [routingProtocol] = ma.stringTuples();
const host = routingProtocol?.[1];

return (
host === undefined || host.startsWith("nox-") || host.startsWith("10.50.10")
);
};

const DENY_CONDITIONS = [DOCKER_NOX_DENY_CONDITION];
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just use camelCase for these variables, they are not really constants

const host = routingProtocol?.[1];

return (
host === undefined || host.startsWith("nox-") || host.startsWith("10.50.10")
Copy link
Contributor

@shamsartem shamsartem Jan 19, 2024

Choose a reason for hiding this comment

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

I am understanding correctly that when this is true - then dialing this multiaddr is forbidden?
why specifically nox- and 10.50.10. Would be nice to leave the comment about that

Copy link
Member

Choose a reason for hiding this comment

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

there should be a more general way to handle that

Copy link
Member

Choose a reason for hiding this comment

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

what if non-available addresses returned in real life, not in tests?

@akim-bow akim-bow requested a review from shamsartem January 19, 2024 09:14
Copy link
Contributor

@shamsartem shamsartem left a comment

Choose a reason for hiding this comment

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

nice job thanks

@akim-bow akim-bow enabled auto-merge (squash) January 19, 2024 09:23
@akim-bow akim-bow merged commit a8a1473 into master Jan 19, 2024
9 checks passed
@akim-bow akim-bow deleted the DXJ-628 branch January 19, 2024 09:25
@fluencebot fluencebot mentioned this pull request Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants