feat(l2ps-messaging): bring instant-messaging server up onto stabilisation#936
Open
Shitikyan wants to merge 6 commits into
Open
feat(l2ps-messaging): bring instant-messaging server up onto stabilisation#936Shitikyan wants to merge 6 commits into
Shitikyan wants to merge 6 commits into
Conversation
…protocol - Implemented unit tests for message hashing, encryption, and decryption functionalities. - Added integration tests for WebSocket server handling peer registration, messaging, discovery, and error handling. - Defined protocol types for messaging, including message envelopes and server/client message structures. - Enhanced setup scripts for zk keys and verification keys for improved reliability. - Updated datasource to include L2PS messaging entities for database integration.
…ry and public key requests
…or L2PS submissions
After rebasing onto stabilisation, four upstream changes broke the messaging-server typing on the rebased commits. None of these are behavioural — they only realign the types so `tsc` no longer rejects the file. - `Bun.serve()` and `Bun.Server` dropped the outer `WSData` generic. Type the inner `ServerWebSocket<WSData>` cast at the dispatch boundary; the upgrade() handler still seeds the data shape so the cast is sound. Applied both in the server and the integration test. - `L2PSMessagingService` writes `status: "failed"` to the entity when an L2PS submission errors. The on-stabilisation `L2PSMessage.status` enum did not include `"failed"`. Adding it preserves the messaging side's failure path without touching any other consumer. - `TxFee` gained a required `rpc_address` field on stabilisation. The messaging tx assembler now provides an empty default; messaging txs are not RPC-fee'd, so an empty string is the right neutral. - `indexState` type literal did not declare the three new messaging fields (`L2PS_MESSAGING_ENABLED`, `L2PS_MESSAGING_PORT`, `l2psMessagingServer`) the messaging boot/shutdown code reads. Add them to the type so the initialiser and the start/stop wiring type-check. Net effect on full-project `tsc --noEmit --skipLibCheck`: baseline (origin/stabilisation): 19 pre-existing errors this branch: 15 — 0 new, 4 fixed The 4 fixed are the broken `./L2PSMessagingService` and `./types` imports plus 2 stale `Bun.Server` typing errors on the partially- shipped server file that the rebase replaces with the complete one. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Contributor
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Contributor
|
Warning Review limit reached
More reviews will be available in 7 minutes and 25 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughThis PR introduces a complete L2PS-backed peer-to-peer messaging system with WebSocket server, encrypted message delivery, offline queuing with rate limiting, message history retrieval, and peer discovery. The implementation includes cryptographic primitives, a service layer that submits messages as L2PS transactions, comprehensive test coverage (unit, integration, and end-to-end), and conditional runtime enablement via environment variables. ChangesL2PS Messaging Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR restores the L2PS instant-messaging WebSocket sidecar onto the
Confidence Score: 4/5The core implementation is solid, but any registered peer can request arbitrarily large conversation history, causing unbounded DB scans — that path needs a server-side cap before the feature goes live. The history handler passes the client-supplied src/features/l2ps-messaging/L2PSMessagingServer.ts — specifically the history handler at line 317 where the client-controlled limit reaches the database uncapped. Important Files Changed
Sequence DiagramsequenceDiagram
participant C as Client
participant S as L2PSMessagingServer
participant SVC as L2PSMessagingService
participant DB as Postgres (l2ps_messages)
participant L2PS as L2PS Mempool
C->>S: "register {publicKey, l2psUid, proof}"
S->>S: verify proof (ucrypto.verify)
S->>S: store peer in peers Map
S->>DB: getQueuedMessages(toKey, l2psUid)
DB-->>S: queued[]
S->>C: registered + queued messages
S->>DB: markDelivered(ids)
C->>S: "send {to, encrypted, messageHash}"
S->>SVC: processMessage(from, to, l2psUid, ...)
SVC->>DB: isDuplicate check
SVC->>DB: repo.save(msg)
SVC->>L2PS: addTransaction + execute
L2PS-->>SVC: "{success, txHash}"
SVC->>DB: repo.update(status)
SVC-->>S: "{success, l2psTxHash}"
S->>C: message_sent / message_queued
C->>S: "history {peerKey, proof, limit?}"
S->>S: verify proof (ucrypto.verify)
S->>SVC: getHistory(myKey, peerKey, l2psUid, before, limit)
SVC->>DB: QueryBuilder (.take(limit+1))
DB-->>SVC: StoredMessage[]
SVC-->>S: "{messages, hasMore}"
S->>C: history_response
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
81
to
82
| NetworkUpgradeVote, | ||
| // Hard-fork bookkeeping (P3b) |
There was a problem hiding this comment.
l2ps_messages
L2PSMessage is registered as an entity but the datasource has synchronize: false, so the table will not be auto-created. Every path that calls dataSource.getRepository(L2PSMessage) — processMessage, getQueuedMessages, markDelivered, getHistory — will throw relation "l2ps_messages" does not exist at runtime. A TypeORM migration file that creates the table (with its columns and indexes) needs to be added and run before this feature can be used.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
scripts/l2ps-messaging-test.ts (1)
189-191: ⚡ Quick winUse actual encryption in the E2E payload instead of base64-encoding plaintext.
Current payload construction labels data as encrypted but sends readable plaintext-as-base64, so this script does not validate the encryption path end-to-end.
Also applies to: 207-209
🤖 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 `@scripts/l2ps-messaging-test.ts` around lines 189 - 191, The payload currently sets ciphertext and nonce by base64-encoding plaintext (the ciphertext and nonce fields) which falsely labels data as encrypted; replace these Buffer.from("...").toString("base64") assignments with a real encryption call (e.g., call your existing encrypt function or use the chosen crypto primitive) so that ciphertext is the base64-encoded result of encrypt(plaintext, generatedNonce, recipientKey, senderKey) and nonce is the base64-encoded nonce produced by that encryption; update both occurrences (the ciphertext/nonce assignments at the shown block and the similar block at 207-209) to use the encryption function and ensure outputs are encoded to base64 for transport.src/features/l2ps-messaging/tests/L2PSMessagingService.test.ts (1)
37-279: 🏗️ Heavy liftMost tests here don’t exercise
L2PSMessagingServicebehavior.These cases mostly validate local Sets/arrays/literals, so regressions in real paths (
processMessage, dedupe race handling, status updates, quota rollback, repository calls) remain untested.🤖 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 `@src/features/l2ps-messaging/tests/L2PSMessagingService.test.ts` around lines 37 - 279, Tests in this file mostly assert local data structures and don't call the real L2PSMessagingService, so add integration/unit tests that instantiate or mock L2PSMessagingService and exercise its real paths (e.g., call L2PSMessagingService.processMessage, simulate dedupe race conditions, verify status updates and quota rollback, and assert repository interactions). Specifically, write tests that use the L2PSMessagingService class (or a test harness) to send messages through processMessage, inject/mocks for the repository and network layers to observe calls (repository methods, l2ps submission), trigger duplicate-hash scenarios to validate dedupe handling, and assert lifecycle transitions (queued→sent, l2ps_pending→l2ps_batched→l2ps_confirmed) and quota rollback behavior.
🤖 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 `@scripts/l2ps-messaging-test.ts`:
- Around line 138-145: You're sending frames (wsAlice.send(...),
wsBob.send(...), etc.) before you attach the corresponding response listeners
(waitForAny, waitFor), which can cause a race where a fast server reply is
missed; fix by creating and awaiting the promise returned by waitForAny/waitFor
(install the listener) first and only then calling the matching
send(frame(...)); update all occurrences that follow this pattern (e.g., the
register flow using wsAlice.send + waitForAny, and the other send/wait pairs
elsewhere that use waitFor/waitForAny and frame) so listeners are registered
before each send.
In `@src/features/l2ps-messaging/crypto.ts`:
- Around line 24-25: The current ambiguous concatenation `const input =
`${from}:${to}:${content}:${timestamp}`` can create colliding preimages; replace
constructing `input` with a canonical serialization (e.g., build an explicit
object { from, to, content, timestamp } and use JSON.stringify on it to
guarantee a deterministic field order, or use a length-prefixed encoding for
each field) and then pass that serialized string into Hashing.sha256(input);
update the code where `input` is created (the variable used immediately before
Hashing.sha256) and ensure any binary `content` is safely encoded (e.g., base64)
before serialization.
In `@src/features/l2ps-messaging/entities/L2PSMessage.ts`:
- Around line 10-20: The entity only has single-column indexes (fromKey, toKey,
l2psUid) but the read paths getQueuedMessages and getHistory filter/sort on
multi-column patterns; add composite indexes on the L2PSMessage entity that
match those query predicates/sorts (for example add
`@Index`(["toKey","processed","scheduledAt"]) and `@Index`(["l2psUid","createdAt"])
or whatever exact column combinations getQueuedMessages and getHistory use) so
the database can use index-only/covering plans; place the `@Index`(...) decorators
at the class level in the L2PSMessage entity referencing the existing property
names (fromKey, toKey, l2psUid and the timestamp/processed fields used by the
queries) and ensure names/order reflect the query WHERE and ORDER BY patterns
(also add the same composite indexes covering the columns referenced near lines
35-36).
In `@src/features/l2ps-messaging/L2PS_MESSAGING_QUICKSTART.md`:
- Line 26: In src/features/l2ps-messaging/L2PS_MESSAGING_QUICKSTART.md there are
unlabeled fenced code blocks that trigger markdownlint MD040; update each
unlabeled triple-backtick fence (the ones currently at the five reported
locations) to include an appropriate language identifier (e.g., ```bash,
```json, ```ts, ```yaml, or ```text as applicable to the snippet) so all code
fences are labeled and markdownlint MD040 is satisfied.
- Around line 275-283: The lifecycle status table in
L2PS_MESSAGING_QUICKSTART.md is missing the `failed` status; add a new table row
for `failed` (e.g., | ❌ **failed** | Submission to L2PS failed / encountered an
error |) so the table of Status meanings includes the current `failed` state and
a concise description to help operators troubleshoot L2PS submission failures;
update the table alongside the existing entries (⚡ delivered, 📬 queued, ✉️
sent, 🔄 l2ps_pending, 📦 l2ps_batched, ✓ l2ps_confirmed) to keep statuses
consistent with runtime values.
In `@src/features/l2ps-messaging/L2PSMessagingServer.ts`:
- Around line 305-308: The catch block after ucrypto.verify in
L2PSMessagingServer currently swallows errors; change it to capture the
exception (e.g. catch (error)) and log the failure before calling this.sendError
so operators see the verification failure details (use the same logging approach
used in the register handler that logs `${error}`), then send the
"INVALID_PROOF" response; reference: ucrypto.verify, the surrounding catch
block, and this.sendError to locate where to add the logging.
In `@src/features/l2ps-messaging/L2PSMessagingService.ts`:
- Around line 166-175: The code uses parallelNetworks.encryptTransaction(...)
and then passes encryptedTx (and implicitly encryptedTx.hash) into downstream
writes like L2PSMempool.addTransaction; add a defensive guard that verifies
encryptedTx and encryptedTx.hash are defined (and of expected type) before
calling L2PSMempool.addTransaction (and any similar writes in the 183-207
region), and if the check fails return/log/throw a clear error or skip the
mempool write so undefined isn’t persisted; update any callers that assume
non-null to handle the validation result instead of using non-null assertions.
- Around line 240-244: markDelivered currently updates message status to "sent"
but doesn't release the offline quota allocated in processMessage, so senders
remain capped; modify markDelivered to first load the messages being updated
(use dataSource.getRepository(L2PSMessage) to findByIds or find({ where: { id:
In(messageIds) } }), group them by senderId, and for each sender
decrement/release their offline quota by the number of messages delivered (call
the existing quota-release function if one exists, or update the sender/offline
quota counter in the Sender/User repo inside the same transaction), then update
the messages' status to "sent"—ensure this runs in a transaction to keep quota
and message state consistent.
In `@src/features/l2ps-messaging/tests/L2PSMessagingService.test.ts`:
- Around line 84-88: The test fixture for transaction_fee in
L2PSMessagingService.test.ts is missing the rpc_address field that the service
includes in payloads; update the fixture object named transaction_fee to include
rpc_address (matching the shape the service builds, e.g., the same key/type used
by L2PSMessagingService when constructing transaction_fee) so the test mirrors
the real payload and catches schema drift.
In `@src/index.ts`:
- Around line 192-194: The env-parsed L2PS_MESSAGING_PORT can be non-numeric or
out-of-range causing startup failures; update the initialization logic that sets
L2PS_MESSAGING_PORT (and any other parseInt usages around L2PS_MESSAGING_ENABLED
/ l2psMessagingServer) to validate the parsed value is a finite integer within
the valid TCP port range (1–65535), and if not, fall back to the default 3006
and emit a warning/log message via the existing logger; ensure downstream code
that probes/starts the sidecar uses this validated value so invalid env values
do not propagate.
---
Nitpick comments:
In `@scripts/l2ps-messaging-test.ts`:
- Around line 189-191: The payload currently sets ciphertext and nonce by
base64-encoding plaintext (the ciphertext and nonce fields) which falsely labels
data as encrypted; replace these Buffer.from("...").toString("base64")
assignments with a real encryption call (e.g., call your existing encrypt
function or use the chosen crypto primitive) so that ciphertext is the
base64-encoded result of encrypt(plaintext, generatedNonce, recipientKey,
senderKey) and nonce is the base64-encoded nonce produced by that encryption;
update both occurrences (the ciphertext/nonce assignments at the shown block and
the similar block at 207-209) to use the encryption function and ensure outputs
are encoded to base64 for transport.
In `@src/features/l2ps-messaging/tests/L2PSMessagingService.test.ts`:
- Around line 37-279: Tests in this file mostly assert local data structures and
don't call the real L2PSMessagingService, so add integration/unit tests that
instantiate or mock L2PSMessagingService and exercise its real paths (e.g., call
L2PSMessagingService.processMessage, simulate dedupe race conditions, verify
status updates and quota rollback, and assert repository interactions).
Specifically, write tests that use the L2PSMessagingService class (or a test
harness) to send messages through processMessage, inject/mocks for the
repository and network layers to observe calls (repository methods, l2ps
submission), trigger duplicate-hash scenarios to validate dedupe handling, and
assert lifecycle transitions (queued→sent,
l2ps_pending→l2ps_batched→l2ps_confirmed) and quota rollback behavior.
🪄 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: ee1264d1-2f1d-476e-a849-ed4d6ffbc4e4
📒 Files selected for processing (16)
.env.examplescripts/l2ps-messaging-test.tssrc/features/l2ps-messaging/L2PSMessagingServer.tssrc/features/l2ps-messaging/L2PSMessagingService.tssrc/features/l2ps-messaging/L2PS_MESSAGING_QUICKSTART.mdsrc/features/l2ps-messaging/crypto.tssrc/features/l2ps-messaging/entities/L2PSMessage.tssrc/features/l2ps-messaging/index.tssrc/features/l2ps-messaging/tests/L2PSMessagingServer.test.tssrc/features/l2ps-messaging/tests/L2PSMessagingService.test.tssrc/features/l2ps-messaging/tests/crypto.test.tssrc/features/l2ps-messaging/tests/integration.test.tssrc/features/l2ps-messaging/types.tssrc/features/zk/scripts/setup-zk.tssrc/index.tssrc/model/datasource.ts
Migration & schema - Add `CreateL2PSMessagesTable1781556000000` so the `l2ps_messages` table actually exists when the feature flag is flipped on. With `synchronize: false`, the @entity decorator was registering the type but not creating the table; every processMessage / getQueuedMessages / markDelivered / getHistory call would have thrown `relation "l2ps_messages" does not exist`. The migration also adds the two composite indexes the hot read paths actually need (`(to_key, l2ps_uid, status, timestamp)` for getQueuedMessages and `(l2ps_uid, timestamp DESC)` for getHistory). The entity now also declares those composite indexes via @Index for documentation parity. Correctness - crypto.ts: canonicalise the messageHash input via JSON.stringify of the field bag instead of a `${from}:${to}:${content}:${ts}` concatenation. The bare-string form was delimiter-ambiguous — a `from` of `"a:b"` would have collided with a different `to` value on the unique `message_hash` row constraint. - L2PSMessagingService: replace `encryptedTx.hash!` non-null assertions with an explicit shape check that returns a clean SDK error when the wrapper has no hash, preventing literal `undefined` from leaking into mempool status updates and record-transaction writes. - L2PSMessagingService.markDelivered: release the per-sender offline quota slot for every queued → sent transition. Without this, a sender bursting up to MAX_OFFLINE_MESSAGES_PER_SENDER stayed at the cap forever after recipients drained the queue. Diagnostics - L2PSMessagingServer (history-proof verify): log the underlying error before collapsing it into the generic INVALID_PROOF response. Auth failures are operationally interesting; the silent catch left operators blind. Process-init safety - setup-zk.ts: defer `execSync("which npx")` from top-level to a lazy `getNpx()` helper. Eager resolution at import time crashed every consumer of this module on require in CI / minimal containers where npx is not on PATH; the lazy form only fails when callers actually need npx and surfaces a clear error. - src/index.ts: validate `L2PS_MESSAGING_PORT` before stamping it onto indexState. A non-integer / out-of-range env now falls back to the default with a warn, instead of leaking `NaN` into `Bun.serve()` and failing later at bind time with an opaque error. Cleanup & docs - L2PSMessagingService: drop the unused `Hashing` and `getSharedState` imports. - L2PSMessagingService.test.ts: keep the test transaction-fee fixture aligned with the service payload shape (`rpc_address`). - L2PS_MESSAGING_QUICKSTART.md: label every opening fenced block with `text` (MD040) and add the `failed` status to the lifecycle table. - scripts/l2ps-messaging-test.ts: register response listeners before `send()` for the three remaining send-then-await sites (register x2, discover) so the script does not hang on a fast server reply. Net `tsc --noEmit --skipLibCheck` delta on this branch: baseline `origin/stabilisation`: 19 pre-existing errors this branch: 15 — 0 net new, 4 fixed Co-Authored-By: Claude Opus 4.7 <[email protected]>
Both functions were flagged CRITICAL by SonarCloud on PR #936 with cognitive complexity above the 15 threshold. The refactors are behaviour-preserving — just extraction: L2PSMessagingService.processMessage (was 17) Split the chained `dedup → quota → persist → submit → update` flow into four private helpers (`isDuplicate`, `reserveOfflineQuota`, `persistMessage`, `handleSubmissionFailure`) so the orchestrator body is a flat sequence of early-returns. Quota release stays paired with quota reservation; failure-rollback stays paired with the L2PS submit branch. JSDoc on the orchestrator + on the new helpers documents the invariants each step holds. integration.test.ts websocket dispatcher (was 23) Lift the per-`frame.type` case bodies into top-level test helpers (`parseIncomingFrame`, `dispatchFrame`, `handleRegister`, `handleSend`, `handleDiscover`, `handleRequestPublicKey`). The inline message callback now does only the ws-shape narrowing and a two-line parse + dispatch. `tsc --noEmit --skipLibCheck` delta unchanged on this branch baseline `origin/stabilisation`: 19 pre-existing errors this branch: 15 — 0 net new Co-Authored-By: Claude Opus 4.7 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased follow-up to #686 (which was opened in March against the old
testnetbase and has been sitting cold for 3 months). The three original feat commits are cherry-picked verbatim onto currentstabilisation; a fourth commit re-aligns the typing where stabilisation moved on (Bun.Server generic removed,L2PSMessage.statusenum,TxFee.rpc_address,indexStateliteral type).Closes #686.
What this brings up
src/features/l2ps-messaging/L2PSMessagingServer.ts— Bun WebSocket server for real-time L2PS-backed messaging (fully restored; the stale partial file that was left in stabilisation is replaced)L2PSMessagingService.ts— registration / discovery / history / pubkey lookup + L2PS-tx submissioncrypto.ts— message hashing, encryption, decryption helpersentities/L2PSMessage.ts— Postgres entity for persistence + datasource registrationtests/— unit tests for crypto + server + service + integration round-tripL2PS_MESSAGING_QUICKSTART.md— protocol quickstartL2PS_MESSAGING_ENABLEDenv + configurableL2PS_MESSAGING_PORTsrc/index.tsWhy now
Client (PATH-OS / RandomBlock) asked specifically whether L2PS supports messaging for DACS. The substrate primitives are in place; this PR turns them into a usable endpoint.
Type-check delta
origin/stabilisationTest plan
bun testover the three new suites (crypto / server / service) oncenode_modulesresolves on stabilisationL2PS_MESSAGING_ENABLED=trueon the dev nodes, runscripts/l2ps-messaging-test.tsround-tripsrc/index.tsshutdown path)Out of scope
ChannelMessageenvelopes as L2PS-encrypted transactions instead of via a WebSocket sidecar). Likely worth doing as a follow-up so PATH-OS has both options.🤖 Generated with Claude Code
Summary by CodeRabbit