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

client: update multiaddr dep #3384

Merged
merged 11 commits into from Apr 29, 2024
Merged

client: update multiaddr dep #3384

merged 11 commits into from Apr 29, 2024

Conversation

gabrocheleau
Copy link
Contributor

This PR updates to the latest multiaddr version.

Resolves #3370

@gabrocheleau gabrocheleau marked this pull request as ready for review April 28, 2024 18:25
@@ -30,6 +22,13 @@ type ENRTreeValues = {
domain: string
}

// Copied over from the multiaddr repo: https://github.com/multiformats/js-multiaddr/blob/main/src/convert.ts
// TODO: Can we import this directly from the multiaddr repo? Doing so makes CI fail
Copy link
Contributor

Choose a reason for hiding this comment

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

You can import the convert method from import { convert } from '@multiformats/multiaddr/convert (since multiaddr now uses an exports map in package.json) but I'm all for reducing our exposure to upstream dependencies, especially when it's for a 3 line helper. Will suggest we just remove this TODO and move forward

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 did try doing that but it seemed to error in the CI still, anyhow I agree that removing the dep altogether is better.

acolytec3
acolytec3 previously approved these changes Apr 29, 2024
Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM

@acolytec3 acolytec3 merged commit 5103318 into master Apr 29, 2024
33 of 34 checks passed
@acolytec3 acolytec3 deleted the client/update-multiaddr branch April 29, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update multiaddr
2 participants