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: make local peer id optional #440

Merged
merged 2 commits into from
Aug 13, 2024
Merged

Conversation

achingbrain
Copy link
Collaborator

@achingbrain achingbrain commented Aug 9, 2024

[email protected] will remove the localPeer argument from the secureInbound and secureOutbound methods of the ConnectionEncrypter interface - see libp2p/js-libp2p#2304.

Unfortunately the js-libp2p monorepo has a depdendency on this module so the change cannot be released until this module is compatible... with the unreleased change.

This PR tests the first argument to secureInbound and secureOutbound to ensure that it is actually a PeerId. If not it shuffles all the arguments along by one place. This makes @chainsafe/libp2p-noise compatible with both [email protected] and [email protected].

This PR can be reverted and the first argument removed once [email protected] is released.

`[email protected]` will remove the `localPeer` argument from the
`secureInbound` and `secureOutbound` methods of the
`ConnectionEncrypter` interface.

Unfortunately the `js-libp2p` monorepo has a depdendency on this
module so the change cannot be released until this module is
compatible... with the unreleased change.

This PR tests the first argument to `secureInbound` and
`secureOutbound` to ensure that it is actually a `PeerId`. If
not it shuffles all the arguments along by one place.

This PR can be reverted and the first argument removed once
`[email protected]` is released.
@wemeetagain
Copy link
Member

Won't there still be type errors since the interfaces don't match?

@achingbrain
Copy link
Collaborator Author

I think there will be breakage after the v2 release:

  • @chainsafe/libp2p-noise currently resolves to the @libp2p/interface in the monorepo
  • We have "skipLibCheck": true in the js-libp2p monorepo tsconfig.json so it doesn't check the types of @chainsafe/libp2p-noise
  • When we release v2, @chainsafe/libp2p-noise will resolve to the older @libp2p/interface version
  • At that point I think tsc will go 💥

@achingbrain
Copy link
Collaborator Author

achingbrain commented Aug 9, 2024

Maybe we'll need to do a major here shortly after the v2 release that updates the interface version, then a follow up that upgrades libp2p to the new noise major.

There will be a short window where things are broken but I'm not sure how else we can resolve the circular dependency, other than adding noise to the libp2p monorepo.

I am open to ideas 😅

@wemeetagain
Copy link
Member

wemeetagain commented Aug 9, 2024

Can we cut a breaking release here that relies on a 2.0-compatible commit of libp2p/interface? then after 2.0 is released, rely on the official upstream again.

Eg:

  • publish 16.0 here that relies on commit xxxx-2.0 of libp2p (this could additionally be published with a next or beta dist-tag)
  • publish libp2p 2.0 relying on noise 16
  • publish 16.0.1 that relies on upstream libp2p 2.0 (and publish with the normal dist-tag)

@achingbrain
Copy link
Collaborator Author

achingbrain commented Aug 9, 2024

Actually I wonder if we could use method overloading to be compatible with both versions of the interface?

Deps would get duplicated but tsc should be happy.

@wemeetagain
Copy link
Member

method overloading

worth a shot, should be able to be tested locally for build errors

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

LGTM

@achingbrain achingbrain merged commit 5f92b50 into master Aug 13, 2024
16 checks passed
@achingbrain achingbrain deleted the fix/make-local-peer-id-optional branch August 13, 2024 13:21
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