feat: split transport-libp2p into platform-agnostic core and per-platform packages#21
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR restructures the transport architecture to separate platform-agnostic logic from platform-specific implementation. The core Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/transport-libp2p/src/transport.ts (1)
286-321:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't drop the relay callback on the first unrelated peer update.
The listener is attached only after
connectToRelay()resolves, and it unregisters on the firstself:peer:updateeven when that event does not contain/p2p/${relayId}/p2p-circuit. If the identify update arrived earlier, or the first update is unrelated,connectedToRelayCallbacknever fires. Keep listening until the matching address appears, and check already-known self addresses before subscribing.Minimal fix for the premature unsubscribe
const handler = ( evt: CustomEvent<{ peer: { addresses: Array<{ multiaddr: { toString(): string } }>; }; }>, ) => { const relayAddress = evt.detail.peer.addresses.find((address) => address.multiaddr .toString() .includes(`/p2p/${relayId}/p2p-circuit`), ); - this.libp2p.removeEventListener("self:peer:update", handler); if (relayAddress) { + this.libp2p.removeEventListener("self:peer:update", handler); connectedToRelayCallback( relayAddress.multiaddr.toString(), relayId, ); - } else { - this.logger.error( - "Received peer update event but found no relay address.", - ); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/transport-libp2p/src/transport.ts` around lines 286 - 321, connectToRelays currently registers a one-shot handler after connectToRelay resolves and immediately removes it on the first self:peer:update even if the update doesn't contain the desired relay address, which can drop the callback; change the logic in connectToRelays/connectToRelay's post-connect block to (1) first inspect this.libp2p.components.peerStore.get(this.libp2p.peerId) or this.libp2p.getMultiaddrs()/self addresses (or whichever API exposes current self addresses) for an existing address containing `/p2p/${relayId}/p2p-circuit` and call connectedToRelayCallback immediately if found, and (2) if not found, register a persistent handler (the handler function referenced in the diff) that only calls connectedToRelayCallback and unregisters itself when it finds the matching relayAddress (do not remove the listener on unrelated updates); use the same relayId/connectedToRelayCallback variable names shown in the diff and removeEventListener only inside the code path where a matching address is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/transport-libp2p-node/src/factories.ts`:
- Around line 11-15: The import of TransportLibp2p/NodeOptions/RelayOptions from
"@peerkit/transport-libp2p" in factories.ts fails because the package's compiled
output (dist/) is missing; run the TypeScript build for the
`@peerkit/transport-libp2p` package (npm run build:dist or the repo's equivalent)
to generate dist/index.js and dist/index.d.ts, and if the package.json is
missing a build script add a "build:dist" script that runs tsc (or the existing
build tool) and ensure the exports field still points to "./dist/index.js" and
"./dist/index.d.ts" (or adjust exports) so imports of TransportLibp2p,
NodeOptions, and RelayOptions resolve correctly; also add the dist build to
CI/prepublish or a prepare script so future installs produce the compiled
output.
- Around line 58-80: Create the TransportLibp2p and let it register protocol
handlers before calling libp2pNode.start(): after calling createLibp2p(...)
instantiate TransportLibp2p(libp2pNode, options) so its constructor/registration
logic (the code that registers the /peerkit/access/v1 and other handlers) runs,
then call await libp2pNode.start(); keep the existing
transport.connectToRelays(options.bootstrapRelays) usage (it can remain
fire-and-forget) but ensure connectToRelays is invoked after start if it relies
on a started node.
In `@packages/transport/package.json`:
- Around line 16-46: The package export mapping in "exports" currently points to
implementations that live in optional peer packages listed under
"peerDependencies" with optional flags in "peerDependenciesMeta", which means
consumers won't get them installed automatically; fix this by either moving the
implementations into this package (update "exports" to point to bundled files
and add/build those files), or change the optional peers into regular
"dependencies" so npm will install them for consumers (remove entries from
"peerDependencies"/"peerDependenciesMeta" and add them to "dependencies"), or
add clear install documentation in README explaining that one of the peer
packages must be installed; update "devDependencies" only if you keep them as
dev-time copies for build/test.
In `@packages/transport/tsconfig.json`:
- Around line 3-6: Add "composite": true to the tsconfig's compilerOptions so
the package becomes a composite project; update the existing compilerOptions
object (the same block that contains rootDir and outDir) in the tsconfig for
this package to include composite: true alongside rootDir: "src" and outDir:
"dist".
---
Outside diff comments:
In `@packages/transport-libp2p/src/transport.ts`:
- Around line 286-321: connectToRelays currently registers a one-shot handler
after connectToRelay resolves and immediately removes it on the first
self:peer:update even if the update doesn't contain the desired relay address,
which can drop the callback; change the logic in
connectToRelays/connectToRelay's post-connect block to (1) first inspect
this.libp2p.components.peerStore.get(this.libp2p.peerId) or
this.libp2p.getMultiaddrs()/self addresses (or whichever API exposes current
self addresses) for an existing address containing `/p2p/${relayId}/p2p-circuit`
and call connectedToRelayCallback immediately if found, and (2) if not found,
register a persistent handler (the handler function referenced in the diff) that
only calls connectedToRelayCallback and unregisters itself when it finds the
matching relayAddress (do not remove the listener on unrelated updates); use the
same relayId/connectedToRelayCallback variable names shown in the diff and
removeEventListener only inside the code path where a matching address is found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9bc6b9b3-296f-4d47-a0df-96444ce89756
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
package.jsonpackages/transport-libp2p-node/package.jsonpackages/transport-libp2p-node/src/factories.tspackages/transport-libp2p-node/src/index.tspackages/transport-libp2p-node/tests/access.test.tspackages/transport-libp2p-node/tests/agents.test.tspackages/transport-libp2p-node/tests/integration.test.tspackages/transport-libp2p-node/tests/message.test.tspackages/transport-libp2p-node/tests/relay.test.tspackages/transport-libp2p-node/tests/tsconfig.jsonpackages/transport-libp2p-node/tests/util.tspackages/transport-libp2p-node/tsconfig.jsonpackages/transport-libp2p-node/vitest.config.tspackages/transport-libp2p-react-native/package.jsonpackages/transport-libp2p-react-native/src/index.tspackages/transport-libp2p-react-native/tsconfig.jsonpackages/transport-libp2p/package.jsonpackages/transport-libp2p/src/index.tspackages/transport-libp2p/src/transport.tspackages/transport/package.jsonpackages/transport/src/index.node.tspackages/transport/src/index.rn.tspackages/transport/tsconfig.jsontsconfig.jsontypedoc.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/transport/package.json`:
- Around line 16-27: The package.json "exports" root currently only provides
platform-specific entries ("react-native" and "default"); add a canonical root
export mapping to "./dist/index.js" and "./dist/index.d.ts" so consumers can
import the package without platform conditions. Update the top-level exports
entry (the object under ".") to include a plain default entry pointing "import"
to "./dist/index.js" and "types" to "./dist/index.d.ts" while keeping the
existing "react-native" and "default" conditional mappings intact (so keep the
existing "react-native" and "default" keys such as the current
"./dist/index.rn.js", "./dist/index.node.js", etc.). Ensure the new canonical
paths are present alongside the existing conditional entries in the same exports
object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b911df63-3051-4bdc-89b7-c4cc6835e936
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.github/workflows/test.ymlpackages/transport-libp2p-node/src/factories.tspackages/transport/package.jsonpackages/transport/tsconfig.json
✅ Files skipped from review due to trivial changes (1)
- packages/transport/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/transport-libp2p-node/src/factories.ts
jost-s
left a comment
There was a problem hiding this comment.
Looking very good to me! Really nice, this means that we can have a streamlined build process for mobile and desktop devices. Depending on whether an app is then compiled with react native activated, the corresponding transport is compiled in. Is that correct?
My requests are around naming and docs.
d14de9f to
de8c673
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/transport-libp2p/src/transport.ts (1)
341-377:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep waiting until the relay-circuit address actually appears.
This handler unregisters on the first
self:peer:update, even if that event does not contain/p2p/${relayId}/p2p-circuit. With multiple relays or incremental identify updates,connectedToRelayCallbackcan be lost permanently even though the relay connection succeeded. At minimum, only remove the listener after finding the matching address; ideally also check the current self addresses before subscribing so an already-emitted update is not missed.Proposed minimal fix
const handler = ( evt: CustomEvent<{ peer: { addresses: Array<{ multiaddr: { toString(): string } }>; }; }>, ) => { const relayAddress = evt.detail.peer.addresses.find((address) => address.multiaddr .toString() .includes(`/p2p/${relayId}/p2p-circuit`), ); - this.libp2p.removeEventListener("self:peer:update", handler); if (relayAddress) { + this.libp2p.removeEventListener("self:peer:update", handler); connectedToRelayCallback( relayAddress.multiaddr.toString(), relayId, ); - } else { - this.logger.error( - "Received peer update event but found no relay address.", - ); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/transport-libp2p/src/transport.ts` around lines 341 - 377, The current handler in connectToRelays removes its listener on the first "self:peer:update" even when the update lacks the relay circuit address, causing connectedToRelayCallback to be dropped; fix it by first checking the libp2p's current self peer addresses for the `/p2p/${relayId}/p2p-circuit` address before registering the handler, and change the handler logic to only call libp2p.removeEventListener("self:peer:update", handler) after a matching relayAddress is found (keep the listener active across non-matching updates); ensure you still call connectedToRelayCallback(relayAddress.multiaddr.toString(), relayId) once the matching address appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/transport-libp2p-nodejs/src/factories.ts`:
- Around line 107-109: The relay reservations currently disable defaults via
circuitRelayServer(... reservations: { applyDefaultLimit: false }) but do not
add replacement caps; update the reservations config passed to
circuitRelayServer to include explicit Limit values (set Limit.duration to a
finite milliseconds value and Limit.data to a finite byte limit) so each
reservation enforces an upper bound on connection lifetime and per-direction
bytes (adjust concrete values to your policy, e.g., tens of minutes for duration
and a few megabytes for data) and keep applyDefaultLimit: false to avoid
double-applying defaults.
---
Outside diff comments:
In `@packages/transport-libp2p/src/transport.ts`:
- Around line 341-377: The current handler in connectToRelays removes its
listener on the first "self:peer:update" even when the update lacks the relay
circuit address, causing connectedToRelayCallback to be dropped; fix it by first
checking the libp2p's current self peer addresses for the
`/p2p/${relayId}/p2p-circuit` address before registering the handler, and change
the handler logic to only call libp2p.removeEventListener("self:peer:update",
handler) after a matching relayAddress is found (keep the listener active across
non-matching updates); ensure you still call
connectedToRelayCallback(relayAddress.multiaddr.toString(), relayId) once the
matching address appears.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf1180d5-dfd7-4b96-a99d-21ac9b35429b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
.github/workflows/test.ymlpackage.jsonpackages/transport-libp2p-nodejs/package.jsonpackages/transport-libp2p-nodejs/src/factories.tspackages/transport-libp2p-nodejs/src/index.tspackages/transport-libp2p-nodejs/tests/access.test.tspackages/transport-libp2p-nodejs/tests/agents.test.tspackages/transport-libp2p-nodejs/tests/integration.test.tspackages/transport-libp2p-nodejs/tests/message.test.tspackages/transport-libp2p-nodejs/tests/relay.test.tspackages/transport-libp2p-nodejs/tests/tsconfig.jsonpackages/transport-libp2p-nodejs/tests/util.tspackages/transport-libp2p-nodejs/tsconfig.jsonpackages/transport-libp2p-nodejs/vitest.config.tspackages/transport-libp2p-react-native/package.jsonpackages/transport-libp2p-react-native/src/index.tspackages/transport-libp2p-react-native/tsconfig.jsonpackages/transport-libp2p/package.jsonpackages/transport-libp2p/src/index.tspackages/transport-libp2p/src/transport.tspackages/transport/package.jsonpackages/transport/src/index.rn.tspackages/transport/src/index.tspackages/transport/tsconfig.jsontsconfig.jsontypedoc.json
✅ Files skipped from review due to trivial changes (11)
- packages/transport/src/index.ts
- packages/transport-libp2p-nodejs/tsconfig.json
- packages/transport-libp2p-nodejs/package.json
- packages/transport-libp2p-nodejs/vitest.config.ts
- packages/transport-libp2p-react-native/tsconfig.json
- packages/transport/src/index.rn.ts
- typedoc.json
- packages/transport-libp2p-nodejs/src/index.ts
- package.json
- .github/workflows/test.yml
- tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/transport/tsconfig.json
- packages/transport-libp2p-react-native/src/index.ts
- packages/transport-libp2p/src/index.ts
- packages/transport-libp2p-react-native/package.json
- packages/transport-libp2p/package.json
de8c673 to
48f2b20
Compare
48f2b20 to
115bbc4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/transport-libp2p-core/src/transport.ts (1)
341-377:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove handler unregistration into the success path to avoid losing the relay callback.
At line 365,
this.libp2p.removeEventListener("self:peer:update", handler)is called unconditionally on the firstself:peer:updateevent, even if the event does not contain the matching/p2p-circuitaddress. If identify emits an unrelated address first, the handler is removed and the callback is permanently lost—even though the relay connection succeeded. Move the unregistration inside theif (relayAddress)block after invoking the callback, or implement a timeout/cancel mechanism to prevent indefinite listener attachment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/transport-libp2p-core/src/transport.ts` around lines 341 - 377, The event handler registered in connectToRelays (the "handler" added via this.libp2p.addEventListener("self:peer:update", handler) inside connectToRelays) unregisters unconditionally, which can drop the relay callback if the first update has no matching /p2p-circuit address; change the logic so removeEventListener("self:peer:update", handler) is called only after a successful match and after invoking connectedToRelayCallback (i.e., move the unregistration inside the if (relayAddress) branch), and optionally add a timeout/fallback to remove the handler after N milliseconds to avoid leaking listeners if no matching event ever arrives.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/transport-libp2p-core/src/transport.ts`:
- Around line 144-149: The constructor registers handlers (onAccessConnect,
onAgentsConnect) before this.logger is initialized, creating a race if libp2p is
already started; fix by preventing external callers from instantiating with a
started node: make the TransportLibp2p constructor private (or at least
protected) and add static factory methods on TransportLibp2p that create the
instance with an unstarted Libp2p, finish object initialization (including
this.logger) and only then start libp2p and attach handlers, or alternatively
delay registering handlers until after this.logger is set (e.g., move the
handler registration into an init/start() method called by the static
factories); update/remove module-level factory functions and replace them with
the new static methods so callers cannot bypass the protected construction flow.
---
Outside diff comments:
In `@packages/transport-libp2p-core/src/transport.ts`:
- Around line 341-377: The event handler registered in connectToRelays (the
"handler" added via this.libp2p.addEventListener("self:peer:update", handler)
inside connectToRelays) unregisters unconditionally, which can drop the relay
callback if the first update has no matching /p2p-circuit address; change the
logic so removeEventListener("self:peer:update", handler) is called only after a
successful match and after invoking connectedToRelayCallback (i.e., move the
unregistration inside the if (relayAddress) branch), and optionally add a
timeout/fallback to remove the handler after N milliseconds to avoid leaking
listeners if no matching event ever arrives.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3c8dbd6-4eb5-405c-9ea8-b4a7add1e8ab
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
.github/workflows/test.ymlCLAUDE.mdpackage.jsonpackages/transport-libp2p-core/README.mdpackages/transport-libp2p-core/package.jsonpackages/transport-libp2p-core/src/custom-stream.tspackages/transport-libp2p-core/src/frame.tspackages/transport-libp2p-core/src/index.tspackages/transport-libp2p-core/src/transport.tspackages/transport-libp2p-core/tests/frame.test.tspackages/transport-libp2p-core/tsconfig.jsonpackages/transport-libp2p-core/vitest.config.tspackages/transport-libp2p-nodejs/package.jsonpackages/transport-libp2p-nodejs/src/factories.tspackages/transport-libp2p-nodejs/src/index.tspackages/transport-libp2p-nodejs/tests/access.test.tspackages/transport-libp2p-nodejs/tests/agents.test.tspackages/transport-libp2p-nodejs/tests/custom-stream.test.tspackages/transport-libp2p-nodejs/tests/integration.test.tspackages/transport-libp2p-nodejs/tests/message.test.tspackages/transport-libp2p-nodejs/tests/relay.test.tspackages/transport-libp2p-nodejs/tests/tsconfig.jsonpackages/transport-libp2p-nodejs/tests/util.tspackages/transport-libp2p-nodejs/tsconfig.jsonpackages/transport-libp2p-nodejs/vitest.config.tspackages/transport-libp2p-react-native/package.jsonpackages/transport-libp2p-react-native/src/index.tspackages/transport-libp2p-react-native/tsconfig.jsonpackages/transport-libp2p/package.jsonpackages/transport-libp2p/src/index.rn.tspackages/transport-libp2p/src/index.tspackages/transport-libp2p/tsconfig.jsontsconfig.jsontypedoc.json
✅ Files skipped from review due to trivial changes (11)
- packages/transport-libp2p-core/tsconfig.json
- packages/transport-libp2p-core/src/index.ts
- packages/transport-libp2p-core/README.md
- packages/transport-libp2p/src/index.rn.ts
- packages/transport-libp2p-react-native/tsconfig.json
- typedoc.json
- packages/transport-libp2p-nodejs/tsconfig.json
- CLAUDE.md
- packages/transport-libp2p-nodejs/vitest.config.ts
- .github/workflows/test.yml
- packages/transport-libp2p-nodejs/tests/message.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/transport-libp2p-nodejs/src/index.ts
- packages/transport-libp2p-nodejs/tests/integration.test.ts
- packages/transport-libp2p-react-native/package.json
- packages/transport-libp2p-react-native/src/index.ts
- packages/transport-libp2p-nodejs/tests/relay.test.ts
- packages/transport-libp2p-nodejs/package.json
- packages/transport-libp2p-nodejs/src/factories.ts
- packages/transport-libp2p-nodejs/tests/util.ts
…form packages (#15) Move libp2p instance creation out of `@peerkit/transport-libp2p` so the core only owns protocol logic (access, agents, messages). Add `@peerkit/transport-libp2p-node` with TCP + circuit-relay-v2 factories and a stub `@peerkit/transport-libp2p-react-native`. Introduce `@peerkit/transport` facade with conditional `react-native`/`default` exports so consumers import a single package across platforms.
115bbc4 to
fac4a87
Compare
|
✔️ fac4a87 - Conventional commits check succeeded. |
Summary
@peerkit/transport-libp2pso the core only owns protocol logic (access handshake, agents, messages).@peerkit/transport-libp2p-nodewith TCP + circuit-relay-v2 factories (createNode,createRelay).@peerkit/transport-libp2p-react-native— placeholder package. Real implementation tracked in Implement React Native transport via js-libp2p with polyfills #20.@peerkit/transportfacade with conditionalreact-native/defaultexports so consumers import a single package across platforms.Implements the cross-platform split prototype from #15.
Test plan
npm run lintnpx prettier . --checknpm run build(build:dist + build:docs)npm test— 33 tests acrosstransport-libp2p(9) +transport-libp2p-node(24)