-
Notifications
You must be signed in to change notification settings - Fork 442
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
refactor!: move pnet into separate package #2165
Conversation
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.
lgtm, some comments mostly for my learning
psk: swarmKeyBuffer | ||
})() | ||
const protector: ConnectionProtector = { | ||
async protect (connection) { |
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.
since this is just a fake, now, this is a more isolated test to connection-manager's use of protectors, which is good.
do we need to test that the arguments passed to connection protector are correct?
does the protector have tests to confirm it is doing the right thing since we're no longer confirming that as a side-effect of this test?
@@ -3,3 +3,4 @@ export const INVALID_PSK = 'Your private shared key is invalid' | |||
export const NO_LOCAL_ID = 'No local private key provided' | |||
export const NO_HANDSHAKE_CONNECTION = 'No connection for the handshake provided' | |||
export const STREAM_ENDED = 'Stream ended prematurely' | |||
export const ERR_INVALID_PARAMETERS = 'ERR_INVALID_PARAMETERS' |
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.
should this and others be in @libp2p/interface
?
I know we had a discussion about this somewhere..
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.
Yes, some generic codes will reduce duplication but no need to block on this.
@@ -81,7 +80,7 @@ class PreSharedKeyConnectionProtector implements ConnectionProtector { | |||
} | |||
|
|||
if (connection == null) { | |||
throw new CodeError(Errors.NO_HANDSHAKE_CONNECTION, codes.ERR_INVALID_PARAMETERS) | |||
throw new CodeError(Errors.NO_HANDSHAKE_CONNECTION, Errors.ERR_INVALID_PARAMETERS) |
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.
nit: we should normalize on errors|codes.ACTUAL_ERR
and always import a singular object or always do ACTUAL_ERR
also(prob for outside this PR but..), where does CodeError come from, and can we enforce the error types there? if we did, we could use generics on modules and ensure each module has their possible error types enforced too (makes it easier for devs to know what errors can be thrown)
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.
where does CodeError come from
import { CodeError } from '@libp2p/interface/errors'
we could use generics
Maybe, I think this might get quite unwieldy. Feel free to open a PR to discuss?
peerId: await createEd25519PeerId(), | ||
peerRouting, | ||
registrar, | ||
addressManager, | ||
connectionManager, | ||
transportManager, |
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.
the preference for no trailing commas makes this diff so bad...
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.
cries in needless diff lines
cc @achingbrain
e831751
to
c347bf2
Compare
ab4a4b8
to
4f4bc7a
Compare
c347bf2
to
c8b71d8
Compare
0a74bf8
to
74d0522
Compare
74d0522
to
07a0036
Compare
- Move pnet module to a separate package - Related to #1913
Description
Change checklist