feat(l2ps): SR-4 SDK delivery — CCI binding, channel envelope, encryp…#94
Open
Shitikyan wants to merge 4 commits into
Open
feat(l2ps): SR-4 SDK delivery — CCI binding, channel envelope, encryp…#94Shitikyan wants to merge 4 commits into
Shitikyan wants to merge 4 commits into
Conversation
…ted transcript anchor Closes the three 🟡 gaps in DACS-3 CORE §A.4 so `negotiate-rfq` and `negotiate-sealed-envelope` can run end-to-end on a real private channel. The invariant from §0 — in-channel signer == on-chain party — is enforced substrate-side: every signature path here goes through the connected Demos Ed25519 key (NEVER the RSA L2PS subnet key). All signatures are domain-separated (`dacs-binding:v1:`, `dacs-channelmsg:v1:`, `dacs-transcript:v1:`) over RFC 8785 JCS canonical bytes per §B.7. What is shipped: - WI-0 — `identity/cci`: `ClaimReference` + `signWithPrimaryClaim` / `verifyPrimaryClaimSignature`. Required because the brief assumes types that did not yet exist in SDK. - WI-1 — `l2ps/binding`: `L2PSMembershipBinding` + SR-2 anchor + `resolveMember`. Resolver enforces the dual check (signature + SP-owner matches claim address) so an impostor cannot poison a binding name. - WI-2 — `l2ps/channel`: `ChannelMessage` envelope (verbatim §8.3.3), monotonic per-channel sequence + `InMemoryChannelIdRegistry` (CH-6), `ChannelSession` orchestrator, transcript export + audit verifier. - WI-3 — `l2ps/anchor`: `anchorEncryptedTranscript` with the three §8.7 policies. Reuses L2PS subnet AES-GCM (new `encryptBytes`/`decryptBytes` primitives on the L2PS class, mirroring the per-call-nonce + AAD pattern from `encryptTx`). Anchored payload carries a public content hash so non-members can verify tamper-evidence without the key. Also: add `override` to `cause: unknown` in BroadcastFailedError and TransportError — pre-existing TS4114 in main (noImplicitOverride) that was blocking the build hook. New package exports: `@kynesyslabs/demosdk/identity` and `@kynesyslabs/demosdk/identity/cci`. The l2ps barrel now also exposes `binding`, `channel`, `anchor` namespaces. Tests: 98 unit tests (jest) across the four work items — round-trips, tampering detection, replay/forking, policy gating, signing-key mismatches. Integration tests against a live chain are deferred until the dev environment is back up. Linear: DEM-760 (epic), DEM-761/762/763/764 (WI-0..WI-3). Brief: docs/l2ps-sr4-implementation-brief.md in the node repo. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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 17 minutes and 1 second. 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 Cross-Context Identity (CCI) claim parsing and signing, L2PS encrypted-bytes support, membership binding with on-chain anchoring, channel messaging with session state tracking, encrypted transcript anchoring to storage programs, and comprehensive protocol test coverage across all workflows. ChangesCCI and L2PS Secure Messaging Protocol
Sequence Diagram(s)sequenceDiagram
participant App
participant CCI as CCI.signWithPrimaryClaim
participant Demos
participant L2PS as L2PS.encryptBytes
participant Anchor as anchorEncryptedTranscript
participant Chain as StorageProgram
App->>CCI: (claim, payload, demos)
CCI->>Demos: crypto.sign("ed25519", payload)
Demos->>CCI: signature
App->>L2PS: (plaintext bytes)
L2PS->>L2PS: generate 12-byte nonce
L2PS->>L2PS: AES-GCM encrypt with nonce as AAD
L2PS->>App: { ciphertext, tag, nonce, l2ps_uid }
App->>Anchor: (policy, transcript, signer, l2ps, demos)
Anchor->>L2PS: (canonicalized transcript)
L2PS->>Anchor: encrypted bytes + contentHash
Anchor->>CCI: (transcript bytes)
CCI->>Anchor: signature
Anchor->>Chain: deploy StorageProgram with payload
Chain->>Anchor: storage address
Anchor->>App: { anchor, contentHash }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 delivers the four SR-4 work items (WI-0 through WI-3) that close the DACS-3 CORE §A.4 gaps needed for
Confidence Score: 5/5The PR is safe to merge. The cryptographic protocol paths (sign, verify, AES-GCM encrypt/decrypt) are internally consistent, all three previously flagged issues have been addressed, and the 98-unit test suite covers the key round-trips and tamper-detection cases. No functional defects were found in the changed code. The sequence race condition, the malformed-address denial-of-service in the resolver, and the hex utility duplication are all resolved. The two remaining observations are a misleading JSDoc mode name and a minor code-reuse gap — neither affects runtime behavior. The Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant ChannelSession
participant envelope as signChannelMessage
participant signWithPrimaryClaim
participant Demos
Caller->>ChannelSession: open()
ChannelSession->>ChannelSession: register channelId (CH-6)
Caller->>ChannelSession: sendOutgoing(type, body)
ChannelSession->>ChannelSession: ++highestSeen (sync, reserves slot)
ChannelSession->>envelope: signChannelMessage(unsigned)
envelope->>signWithPrimaryClaim: "sign(dacs-channelmsg:v1: || sha256(JCS))"
signWithPrimaryClaim->>Demos: crypto.sign(ed25519, payload)
Demos-->>signWithPrimaryClaim: Ed25519 signature
signWithPrimaryClaim-->>envelope: sigBytes
envelope-->>ChannelSession: ChannelMessage
ChannelSession->>ChannelSession: transcript.push(signed)
Caller->>ChannelSession: receiveIncoming(msg)
ChannelSession->>ChannelSession: channelId + member + seq checks
ChannelSession->>envelope: verifyChannelMessage(msg)
envelope->>signWithPrimaryClaim: verifyPrimaryClaimSignature
signWithPrimaryClaim-->>envelope: true/false
envelope-->>ChannelSession: ok
Caller->>ChannelSession: exportTranscript
ChannelSession-->>Caller: ChannelTranscript
Caller->>L2PS: encryptBytes(JCS(unsignedTranscript))
L2PS-->>Caller: L2PSEncryptedBytes (fresh nonce+AAD)
Caller->>Demos: "signWithPrimaryClaim(dacs-transcript:v1: || hash)"
Demos-->>Caller: TranscriptSignature
Caller->>StorageProgram: deployAnchorSP (mode:public)
StorageProgram-->>Caller: AttestationRef
Reviews (3): Last reviewed commit: "review: address CodeRabbit findings on P..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Two P1 correctness bugs + two P2 cleanups, all caught by the bot review. P1 — `resolveMember` DoS via malformed SP owner src/l2ps/binding/binding.ts `normalizeDemosAddress(sp.owner)` sat outside the existing try-catch, so a single adversarially-crafted storage program registered under our deterministic name would throw and abort the entire resolution loop instead of being skipped. A squatter could DoS every consumer of `resolveMember`. Moved the owner-address comparison inside the same try-catch as the claim-address parse so any malformed candidate is skipped, the loop continues, and a legitimate later candidate still resolves. P1 — `ChannelSession.sendOutgoing` sequence race src/l2ps/channel/session.ts `this.highestSeen` was read on construction and only written after the `await signChannelMessage` yield. Two unawaited concurrent calls would read the same value, sign two messages with identical sequence numbers, and silently violate the anti-replay invariant on the receiver side. Reserve the sequence slot synchronously via `++this.highestSeen` before any await, so concurrent calls get distinct, monotonic sequences. P2 — duplicated hex helpers src/l2ps/utils/hex.ts (new) src/l2ps/binding/canonical.ts, src/l2ps/channel/canonical.ts, src/l2ps/anchor/anchor.ts `bytesToHex` / `signatureToHex` / `signatureFromHex` were copy-pasted identically in three modules. Extracted into a single shared `src/l2ps/utils/hex.ts`; the binding and channel canonical modules re-export the symbols so existing imports keep working unchanged. P2 — `decryptAnchoredTranscript` end-to-end verification src/l2ps/anchor/anchor.ts Previously only the anchor-party transcript signature was verified; individual `ChannelMessage` signatures inside the transcript body were not, despite the JSDoc implying full verification. Pipe the assembled transcript through `verifyTranscript` before returning so every message signature, sequence monotonicity, sender ∈ members, and per-message channelId check is exercised. Doc comment now lists every condition that throws so callers don't have to re-verify. Tests added: - two regression tests for the resolveMember DoS (malformed candidate among valid ones, and all-malformed → null without throw) - two regression tests for the sendOutgoing race (concurrent calls produce distinct + monotonic sequences, both for N=2 and N=5) 102/102 jest tests pass; `tsc --noEmit --skipLibCheck` clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The quality gate flipped to D Reliability because one new test sort called `.sort()` without a compare function on a numeric array — for sequence values up to 9 the alphabetical default coincidentally passed the assertion, but the test would silently break the moment a sequence reached 10. Pass `(a, b) => a - b` so the assertion is meaningful and the reliability rating goes back to A. While here, knock out the major code-smell cluster Sonar surfaced on the SR-4 modules — all mechanical, no behavior change: - Collapse `if (!x || !x.y || x.y.z !== ...)` patterns into optional- chain form (`x?.y?.z !== ...`) in anchor, binding, channel/envelope and channel/transcript verifiers — five sites Sonar flagged as MAJOR. - Switch the two `parseInt(..., 16)` calls in `utils/hex.ts` and `identity/cci/signing.ts` to `Number.parseInt` so the static reference doesn't depend on the global. 102/102 jest tests still pass; `tsc --noEmit --skipLibCheck` clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![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: 9
🧹 Nitpick comments (6)
src/identity/cci/signing.ts (1)
74-78: Clarify UTF-8 requirement for Ed25519 verification (and keep the decode step)
src/identity/cci/signing.ts(lines 74-78) decodespayloadwithTextDecoderbecauseCryptography.verifytakessigned: stringand verifies viaforge.pki.ed25519.verify({ message: signed, encoding: "utf8" }). This matches the existing Ed25519 signing path (src/encryption/unifiedCrypto.tsdecodesdatawithTextDecoderbefore callingCryptography.sign), so the decode isn’t redundant—document thatpayloadmust be valid UTF-8 (and matches the bytes that were signed) in the relevant JSDoc (verifyPrimaryClaimSignature/Cryptography.verify).🤖 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/identity/cci/signing.ts` around lines 74 - 78, Keep the TextDecoder.decode(payload) call in verifyPrimaryClaimSignature/Cryptography.verify path and add JSDoc to both verifyPrimaryClaimSignature and Cryptography.verify (or the verify method implementation) clarifying that the incoming payload must be valid UTF-8 and that the verifier expects the original signed bytes were UTF-8 decoded (so the string passed to forge.pki.ed25519.verify uses encoding: "utf8"); update docs to state the decoded string must exactly match the bytes that were signed and reference the TextDecoder.decode usage in signing (src/encryption/unifiedCrypto.ts) for consistency.src/l2ps/index.ts (1)
2-8: ⚡ Quick winUse the repo alias in the new barrel exports.
These new specifiers extend the public L2PS surface, so it is worth keeping them on the repo’s
@/import contract instead of adding more relative paths here.♻️ Suggested change
-import L2PS from "./l2ps" -import { L2PSConfig, L2PSEncryptedBytes, L2PSEncryptedPayload } from "./l2ps" +import L2PS from "`@/l2ps/l2ps`" +import { + L2PSConfig, + L2PSEncryptedBytes, + L2PSEncryptedPayload, +} from "`@/l2ps/l2ps`" export { L2PS, L2PSConfig, L2PSEncryptedBytes, L2PSEncryptedPayload } -export * as binding from "./binding" -export * as channel from "./channel" -export * as anchor from "./anchor" +export * as binding from "`@/l2ps/binding`" +export * as channel from "`@/l2ps/channel`" +export * as anchor from "`@/l2ps/anchor`"As per coding guidelines,
**/*.{ts,tsx}should “Use@/path aliases instead of relative imports.”🤖 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/l2ps/index.ts` around lines 2 - 8, The barrel exposes new symbols using relative paths; update the imports/exports to use the repo alias (@"//") contract instead of relative imports: import the types from "`@/l2ps`" and export the submodules as "`@/l2ps/binding`", "`@/l2ps/channel`", and "`@/l2ps/anchor`" while keeping exported symbols L2PS, L2PSConfig, L2PSEncryptedBytes, L2PSEncryptedPayload and the named exports binding, channel, anchor unchanged so the public surface uses the "`@/`..." aliases.Source: Coding guidelines
src/l2ps/l2ps.ts (1)
433-437: ⚡ Quick winRemove the new
as anyfrom the encryption path.This cast punches a hole in type coverage right on the new crypto path. Prefer a typed helper or a
createBuffercall that does not need ananyescape hatch.♻️ Suggested change
cipher.update( forge.util.createBuffer( Buffer.from(plaintext).toString('binary'), - 'binary' as any, ), );As per coding guidelines,
**/*.{ts,tsx}should “Ensure full TypeScript type coverage.”🤖 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/l2ps/l2ps.ts` around lines 433 - 437, Remove the "as any" escape by introducing a small typed helper that returns a forge ByteBuffer from the plaintext and use it in the cipher.update call; e.g. add a helper like binaryBufferFromPlaintext(plaintext: string): forge.util.ByteBuffer which calls forge.util.createBuffer(Buffer.from(plaintext).toString('binary'), 'binary') (typed correctly) and replace the inline createBuffer(...) call in the cipher.update invocation with binaryBufferFromPlaintext(plaintext), removing the cast.Source: Coding guidelines
src/l2ps/channel/canonical.ts (2)
40-67: ⚡ Quick winAdd JSDoc comments for helper functions.
Four functions lack JSDoc documentation:
stripChannelMessageSignature,transcriptHashHex,transcriptSigningBytes, andstripTranscriptSignatures. As per coding guidelines, all new methods and functions should use JSDoc format.📝 Suggested JSDoc additions
+/** + * Remove the signature field from a ChannelMessage, returning the unsigned envelope. + */ export function stripChannelMessageSignature( msg: ChannelMessage, ): UnsignedChannelMessage { +/** + * Hex digest of `JCS(transcript_without_signatures)`. Used for transcript signature verification. + */ export function transcriptHashHex( unsigned: UnsignedChannelTranscript, ): string { +/** + * Bytes the transcript signature covers: + * `dacs-transcript:v1:` || transcript_hash_hex + */ export function transcriptSigningBytes( unsigned: UnsignedChannelTranscript, ): Uint8Array { +/** + * Remove the signatures array from a ChannelTranscript, returning the unsigned transcript. + */ export function stripTranscriptSignatures( t: ChannelTranscript, ): UnsignedChannelTranscript {🤖 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/l2ps/channel/canonical.ts` around lines 40 - 67, Add JSDoc comments to the four helper functions so they meet project guidelines: document purpose, parameters, return types, and any important behavior for stripChannelMessageSignature, transcriptHashHex, transcriptSigningBytes, and stripTranscriptSignatures. For each function include a one-line description, `@param` for the input types (ChannelMessage, UnsignedChannelTranscript, ChannelTranscript), and `@returns` describing the returned type/value (UnsignedChannelMessage, hex string, Uint8Array, UnsignedChannelTranscript). Keep descriptions concise and note any encoding/formatting details (e.g., uses canonicalJSONStringify, sha256, TRANSCRIPT_DOMAIN_PREFIX) where relevant.Source: Coding guidelines
69-71: 💤 Low valueConsider using
export...fromfor re-exports.The comment explains these are re-exported for compatibility, but SonarCloud suggests using the more idiomatic
export { signatureFromHex, signatureToHex } from "../utils/hex"syntax instead of separate import + export statements.♻️ Proposed refactor
-// Re-exported from the shared utility so existing imports of -// `signatureFromHex` / `signatureToHex` from this module keep working. -export { signatureFromHex, signatureToHex } +// Re-exported from the shared utility so existing imports of +// `signatureFromHex` / `signatureToHex` from this module keep working. +export { signatureFromHex, signatureToHex } from "../utils/hex"And remove the corresponding imports from line 3:
-import { bytesToHex, signatureFromHex, signatureToHex } from "../utils/hex" +import { bytesToHex } from "../utils/hex"🤖 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/l2ps/channel/canonical.ts` around lines 69 - 71, Replace the current import-then-export pattern for signatureFromHex and signatureToHex with a direct re-export using the `export { signatureFromHex, signatureToHex } from "…"` form: update the module to directly re-export these symbols (signatureFromHex, signatureToHex) from the shared hex utils and remove the now-redundant import statements that originally brought them into this module so only the single `export ... from` remains.Source: Linters/SAST tools
src/l2ps/channel/channelIdRegistry.ts (1)
16-31: ⚡ Quick winAdd JSDoc for
InMemoryChannelIdRegistryclass and methods.The
InMemoryChannelIdRegistryclass and itsregister/hasmethods lack JSDoc documentation. As per coding guidelines, all new methods and functions should use JSDoc format.📝 Suggested JSDoc additions
+/** + * In-memory implementation of ChannelIdRegistry for SDK default usage. + * Production deployments should provide a durable store implementation. + */ export class InMemoryChannelIdRegistry implements ChannelIdRegistry { private readonly seen = new Set<string>() + /** + * Register a channelId, throwing if it was already registered (CH-6). + */ async register(channelId: string): Promise<void> { + /** + * Check if a channelId has been registered. + */ async has(channelId: string): Promise<boolean> {🤖 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/l2ps/channel/channelIdRegistry.ts` around lines 16 - 31, Add JSDoc comments to the InMemoryChannelIdRegistry class and its methods: document the class purpose, the register(channelId: string) method (describe parameter, that it throws if channelId is falsy, and that it throws on duplicate registration / CH-6 violation), and the has(channelId: string): Promise<boolean> method (describe parameter and return value). Use standard JSDoc tags like `@class` or brief description for the class, `@param` for channelId, `@returns` for has and register (Promise<void>), and `@throws` for the error cases; apply these comments directly above the InMemoryChannelIdRegistry class, the register method, and the has method.Source: Coding guidelines
🤖 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 `@src/identity/cci/claim.ts`:
- Around line 3-57: Add JSDoc blocks for each exported function (parseClaimRef,
demosClaimRefForAddress, isDemosClaim, normalizeDemosAddress,
demosAddressFromClaim) describing their purpose, parameters (with types), return
value, and thrown exceptions; place the JSDoc immediately above each function
definition, include `@param`, `@returns`, and `@throws` tags as appropriate (e.g.,
parseClaimRef throws TypeError or Error on malformed input;
normalizeDemosAddress throws Error for non-hex or wrong length), and keep
descriptions concise and consistent with existing coding guidelines.
In `@src/identity/cci/signing.ts`:
- Around line 21-46: Add a JSDoc block for the signWithPrimaryClaim function
that documents the parameters (claim: ClaimReference, payload: Uint8Array,
demos: Demos), the Promise<Uint8Array> return value, and the possible thrown
errors (unsupported scheme from parseClaimRef, "Demos wallet not connected", and
claim mismatch after demosAddressFromClaim/normalizeDemosAddress); briefly
describe behavior (verifies scheme is "demos", checks wallet connection, ensures
connected address matches claim, and uses demos.crypto.sign to sign the payload,
returning the signature via toUint8Array).
- Around line 55-79: Add JSDoc for verifyPrimaryClaimSignature: document the
function purpose, parameters (claim: ClaimReference, payload: Uint8Array,
signature: Uint8Array), return type (boolean), and possible thrown errors
(unsupported scheme, invalid public key length). Mention expected scheme "demos"
and that the public key is a 32-byte Ed25519 key derived via
demosAddressFromClaim and hexAddressToBytes, and note that verification is
performed using Cryptography.verify with the payload decoded as UTF-8. Keep the
JSDoc immediately above the verifyPrimaryClaimSignature declaration.
In `@src/l2ps/anchor/anchor.ts`:
- Around line 37-39: Add JSDoc blocks for the public API functions
anchorProgramName, anchorEncryptedTranscript, decryptAnchoredTranscript, and
verifyAnchorIntegrity: for each function include a one-line description, `@param`
entries describing types and purpose of each parameter (e.g., channelId,
transcript, key, signature, etc.), an `@returns` describing the return value and
its type, and an `@throws` or `@returns` {Promise<...>} note if the function can
reject/errors; ensure the JSDoc matches actual parameter names and types used in
those functions and include brief examples or `@see` references where helpful to
aid IDE tooling and generated docs.
In `@src/l2ps/binding/binding.ts`:
- Around line 61-79: createMembershipBinding currently accepts opts.boundAt
without validating it's a numeric timestamp and verifyMembershipBinding (and any
verifier used by resolveMember) never asserts boundAt is a valid number,
allowing malformed or missing boundAt to pass; update createMembershipBinding to
validate opts.boundAt (if provided) is a finite integer timestamp (e.g.,
Number.isFinite and isInteger or coercible to a safe integer) and throw a clear
Error if not, and update verifyMembershipBinding to assert the incoming
binding.boundAt exists and is a valid numeric timestamp before accepting or
verifying the signature; also add regression tests alongside the tampering tests
that exercise missing, non-numeric, and out-of-range boundAt values to ensure
both creation and verification reject invalid boundAt.
- Around line 29-34: bindingProgramName currently concatenates channelId and
subnetMemberId with ":" which allows collisions (e.g. "a:b":"c" vs "a":"b:c");
change bindingProgramName to produce an injective key by encoding the two parts
unambiguously (for example prefix each part with its length, use a separator
plus escaping, or base64/URL-encode each component) so no two distinct
(channelId, subnetMemberId) pairs map to the same string; update the function
bindingProgramName and ensure callers anchorMembershipBinding and resolveMember
continue to use the new format.
In `@src/l2ps/l2ps.ts`:
- Around line 455-466: The decryptBytes function should validate that
payload.ciphertext, payload.tag, and payload.nonce exist and are strings before
calling forge.util.decode64 to avoid raw base64 errors for storage-loaded JSON;
in decryptBytes (and consider callers like decryptAnchoredTranscript) add checks
that each of ciphertext/tag/nonce is a non-empty string and throw a clear SDK
Error (e.g. "L2PS.decryptBytes: missing or invalid ciphertext/tag/nonce in
L2PSEncryptedBytes") when validation fails, then proceed to perform
forge.util.decode64 and the existing buffer/nonce length checks as before.
In `@src/l2ps/utils/hex.ts`:
- Around line 7-29: Add JSDoc comments to all exported functions: bytesToHex,
signatureToHex, and signatureFromHex; for bytesToHex describe input Uint8Array
and returned hex string, for signatureToHex document that it prefixes "0x" and
returns a hex string for a signature Uint8Array, and for signatureFromHex
document accepted input (allowing optional "0x"/"0X" prefix), validation
behavior (throws Error on invalid hex or odd length), and the returned
Uint8Array; keep descriptions concise and include parameter and return
annotations.
In `@src/tests/l2ps/channel.test.ts`:
- Around line 537-538: The test builds a numeric array `seen` from `m1.sequence`
and `m2.sequence` then calls `.sort()` which performs lexicographic string sort;
change the call on the `seen` array to use a numeric comparator (e.g.,
`.sort((a, b) => a - b)`) so `expect(seen).toEqual([1, 2])` reliably passes for
all numeric values.
---
Nitpick comments:
In `@src/identity/cci/signing.ts`:
- Around line 74-78: Keep the TextDecoder.decode(payload) call in
verifyPrimaryClaimSignature/Cryptography.verify path and add JSDoc to both
verifyPrimaryClaimSignature and Cryptography.verify (or the verify method
implementation) clarifying that the incoming payload must be valid UTF-8 and
that the verifier expects the original signed bytes were UTF-8 decoded (so the
string passed to forge.pki.ed25519.verify uses encoding: "utf8"); update docs to
state the decoded string must exactly match the bytes that were signed and
reference the TextDecoder.decode usage in signing
(src/encryption/unifiedCrypto.ts) for consistency.
In `@src/l2ps/channel/canonical.ts`:
- Around line 40-67: Add JSDoc comments to the four helper functions so they
meet project guidelines: document purpose, parameters, return types, and any
important behavior for stripChannelMessageSignature, transcriptHashHex,
transcriptSigningBytes, and stripTranscriptSignatures. For each function include
a one-line description, `@param` for the input types (ChannelMessage,
UnsignedChannelTranscript, ChannelTranscript), and `@returns` describing the
returned type/value (UnsignedChannelMessage, hex string, Uint8Array,
UnsignedChannelTranscript). Keep descriptions concise and note any
encoding/formatting details (e.g., uses canonicalJSONStringify, sha256,
TRANSCRIPT_DOMAIN_PREFIX) where relevant.
- Around line 69-71: Replace the current import-then-export pattern for
signatureFromHex and signatureToHex with a direct re-export using the `export {
signatureFromHex, signatureToHex } from "…"` form: update the module to directly
re-export these symbols (signatureFromHex, signatureToHex) from the shared hex
utils and remove the now-redundant import statements that originally brought
them into this module so only the single `export ... from` remains.
In `@src/l2ps/channel/channelIdRegistry.ts`:
- Around line 16-31: Add JSDoc comments to the InMemoryChannelIdRegistry class
and its methods: document the class purpose, the register(channelId: string)
method (describe parameter, that it throws if channelId is falsy, and that it
throws on duplicate registration / CH-6 violation), and the has(channelId:
string): Promise<boolean> method (describe parameter and return value). Use
standard JSDoc tags like `@class` or brief description for the class, `@param` for
channelId, `@returns` for has and register (Promise<void>), and `@throws` for the
error cases; apply these comments directly above the InMemoryChannelIdRegistry
class, the register method, and the has method.
In `@src/l2ps/index.ts`:
- Around line 2-8: The barrel exposes new symbols using relative paths; update
the imports/exports to use the repo alias (@"//") contract instead of relative
imports: import the types from "`@/l2ps`" and export the submodules as
"`@/l2ps/binding`", "`@/l2ps/channel`", and "`@/l2ps/anchor`" while keeping exported
symbols L2PS, L2PSConfig, L2PSEncryptedBytes, L2PSEncryptedPayload and the named
exports binding, channel, anchor unchanged so the public surface uses the
"`@/`..." aliases.
In `@src/l2ps/l2ps.ts`:
- Around line 433-437: Remove the "as any" escape by introducing a small typed
helper that returns a forge ByteBuffer from the plaintext and use it in the
cipher.update call; e.g. add a helper like binaryBufferFromPlaintext(plaintext:
string): forge.util.ByteBuffer which calls
forge.util.createBuffer(Buffer.from(plaintext).toString('binary'), 'binary')
(typed correctly) and replace the inline createBuffer(...) call in the
cipher.update invocation with binaryBufferFromPlaintext(plaintext), removing the
cast.
🪄 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: 339f5a4c-51b4-425f-a7b1-ea04796b0445
📒 Files selected for processing (29)
package.jsonsrc/identity/cci/claim.tssrc/identity/cci/index.tssrc/identity/cci/signing.tssrc/identity/cci/types.tssrc/identity/index.tssrc/l2ps/anchor/anchor.tssrc/l2ps/anchor/index.tssrc/l2ps/anchor/types.tssrc/l2ps/binding/binding.tssrc/l2ps/binding/canonical.tssrc/l2ps/binding/index.tssrc/l2ps/binding/types.tssrc/l2ps/channel/canonical.tssrc/l2ps/channel/channelIdRegistry.tssrc/l2ps/channel/envelope.tssrc/l2ps/channel/index.tssrc/l2ps/channel/session.tssrc/l2ps/channel/transcript.tssrc/l2ps/channel/types.tssrc/l2ps/index.tssrc/l2ps/l2ps.tssrc/l2ps/utils/hex.tssrc/tests/identity/cci.test.tssrc/tests/l2ps/anchor.test.tssrc/tests/l2ps/binding.test.tssrc/tests/l2ps/channel.test.tssrc/websdk/BroadcastFailedError.tssrc/websdk/TransportError.ts
CodeRabbit caught one real bug, two defensive-validation gaps, and
asked for JSDoc on every new public export. Substantive fixes first:
bindingProgramName injectivity
src/l2ps/binding/binding.ts
src/l2ps/anchor/anchor.ts
Using `:` as both data and delimiter meant ("a:b","c") and
("a","b:c") collided on the same SP name — `resolveMember` could be
tricked into returning a binding from a sibling context. Percent-encode
the components so the lookup key is one-to-one. Same guard applied to
`anchorProgramName(channelId)` for consistency.
boundAt validation
src/l2ps/binding/binding.ts
`createMembershipBinding` accepted any `boundAt` override; a chain-
loaded binding with `boundAt: NaN` could pass `verifyMembershipBinding`
because the verifier never typed-checked the field. Now reject
non-safe-integer / negative values at create-time and treat them as
structurally invalid at verify-time.
L2PS.decryptBytes payload shape check
src/l2ps/l2ps.ts
Storage-loaded JSON can violate the `L2PSEncryptedBytes` shape at
runtime; without an explicit guard forge would throw an opaque base64
error. Reject any payload whose `ciphertext` / `tag` / `nonce` is not
a string so the caller sees a clear SDK-side message.
JSDoc on every new public export
src/identity/cci/claim.ts, src/identity/cci/signing.ts,
src/l2ps/anchor/anchor.ts, src/l2ps/utils/hex.ts
Convert the prose-style headers into formal JSDoc blocks with @param,
@returns, @throws — required by the project coding guideline. No
behaviour change.
Regression tests added (7 new — total 109/109):
- bindingProgramName: collision-avoidance + backward-compat for
no-special-char ids
- boundAt validation: rejects negative + non-integer override; verifier
rejects NaN at rest
- L2PS.decryptBytes shape: rejects missing fields and non-string types
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
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.
…ted transcript anchor
Closes the three 🟡 gaps in DACS-3 CORE §A.4 so
negotiate-rfqandnegotiate-sealed-envelopecan run end-to-end on a real private channel.The invariant from §0 — in-channel signer == on-chain party — is enforced substrate-side: every signature path here goes through the connected Demos Ed25519 key (NEVER the RSA L2PS subnet key). All signatures are domain-separated (
dacs-binding:v1:,dacs-channelmsg:v1:,dacs-transcript:v1:) over RFC 8785 JCS canonical bytes per §B.7.What is shipped:
WI-0 —
identity/cci:ClaimReference+signWithPrimaryClaim/verifyPrimaryClaimSignature. Required because the brief assumes types that did not yet exist in SDK.WI-1 —
l2ps/binding:L2PSMembershipBinding+ SR-2 anchor +resolveMember. Resolver enforces the dual check (signature + SP-owner matches claim address) so an impostor cannot poison a binding name.WI-2 —
l2ps/channel:ChannelMessageenvelope (verbatim §8.3.3), monotonic per-channel sequence +InMemoryChannelIdRegistry(CH-6),ChannelSessionorchestrator, transcript export + audit verifier.WI-3 —
l2ps/anchor:anchorEncryptedTranscriptwith the three §8.7 policies. Reuses L2PS subnet AES-GCM (newencryptBytes/decryptBytesprimitives on the L2PS class, mirroring the per-call-nonce + AAD pattern fromencryptTx). Anchored payload carries a public content hash so non-members can verify tamper-evidence without the key.Also: add
overridetocause: unknownin BroadcastFailedError and TransportError — pre-existing TS4114 in main (noImplicitOverride) that was blocking the build hook.New package exports:
@kynesyslabs/demosdk/identityand@kynesyslabs/demosdk/identity/cci. The l2ps barrel now also exposesbinding,channel,anchornamespaces.Tests: 98 unit tests (jest) across the four work items — round-trips, tampering detection, replay/forking, policy gating, signing-key mismatches. Integration tests against a live chain are deferred until the dev environment is back up.
Linear: DEM-760 (epic), DEM-761/762/763/764 (WI-0..WI-3).
Brief: docs/l2ps-sr4-implementation-brief.md in the node repo.
Summary by CodeRabbit
Release Notes
New Features
Tests