Skip to content

feat(l2ps): SR-4 WI-B — negotiate-rfq phase logic#96

Closed
Shitikyan wants to merge 1 commit into
feat/sr4-wi-a-channel-transportfrom
feat/sr4-wi-b-negotiate-rfq
Closed

feat(l2ps): SR-4 WI-B — negotiate-rfq phase logic#96
Shitikyan wants to merge 1 commit into
feat/sr4-wi-a-channel-transportfrom
feat/sr4-wi-b-negotiate-rfq

Conversation

@Shitikyan

@Shitikyan Shitikyan commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Stacked on #95 (WI-A). The negotiate-rfq state machine on top of the SR-4 channel substrate.

What

RfqSession (src/l2ps/channel/negotiate.ts) drives offer/counter → accept/reject/abort (CH-5 termination). RFQ bodies are implementation-defined per the SR-4 brief (only sealed-envelope bodies are spec-locked, §8.4.3), so this fixes a minimal schema — proposal carries opaque terms, accept references the accepted proposal's sequence — and the legal-transition rules, staying agnostic to the actual terms.

Pure protocol state machine, transport-agnostic like ChannelSession: wire send to L2PSChannelTransport.send (WI-A) and feed verified inbound envelopes to onIncoming. Channel-layer verification (signature / sender∈members / channelId / sequence) happens before onIncoming, so this only enforces protocol rules. Accepted terms resolve by the exact proposal sequence the accept points at, so a multi-round counter chain settles correctly on both sides.

Tests — 7/7

offer→counter→accept settles agreedTerms on both sides; reject terminates both; accept resolves the referenced proposal across a counter chain; illegal transitions rejected; no action after terminal; inbound accept referencing an unknown proposal throws; impl-defined terms carried opaquely.

Scope

negotiate-sealed-envelope (commit-then-reveal) remains gated on DACS-3 §8.4.3, which is not in the repo. WI-C (transcript anchor on terminal) + WI-D (AgreementDocument co-sign) + WI-E (devnet E2E) follow.

Base

Stacked on #95 so the diff is WI-B only. Rebase onto main once #95 lands.

Summary by CodeRabbit

  • New Features
    • Introduced RFQ (Request for Quote) negotiation capability within channel communications, enabling developers to implement structured bidirectional term proposals, counter-offer exchange, and negotiation completion workflows with full state tracking and proposal history management.

RfqSession drives the request-for-quote state machine: offer/counter
proposals terminating in accept/reject/abort (CH-5 termination). Per
the SR-4 brief the RFQ message bodies are implementation-defined (only
sealed-envelope bodies are spec-locked, §8.4.3), so this fixes a
minimal body schema (proposal carries opaque `terms`; accept references
the accepted proposal's sequence) and the legal-transition rules while
staying agnostic to the actual terms.

Pure protocol state machine — transport-agnostic like ChannelSession:
the caller wires `send` to a transport (e.g. L2PSChannelTransport.send,
WI-A) and feeds verified inbound envelopes to `onIncoming`. By the time
a message reaches onIncoming the channel layer has verified signature /
sender∈members / channelId / sequence, so this layer only enforces
protocol rules (legal transitions, accept referencing an existing
proposal). Resolves accepted terms by the exact proposal sequence the
accept points at, so a multi-round counter chain settles on the right
terms on both sides.

7 tests: offer→counter→accept settles agreedTerms both sides; reject
terminates both; accept resolves the referenced proposal's terms across
a counter chain; illegal transitions rejected (accept/counter with
nothing standing, double offer); no action after terminal; inbound
accept referencing an unknown proposal throws; impl-defined terms
carried opaquely.

negotiate-sealed-envelope remains gated on DACS-3 §8.4.3 (not in repo).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown
Contributor

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Adds src/l2ps/channel/negotiate.ts implementing an SR-4 negotiate-rfq state machine as RfqSession, which tracks negotiation state (open/accepted/rejected/aborted), manages standing proposals, dispatches outbound typed channel messages, and processes verified inbound messages. Protocol types, options, and the class are re-exported from the channel index. A Jest test suite covers the full negotiation lifecycle.

Changes

RFQ Negotiation State Machine

Layer / File(s) Summary
RFQ protocol types and session options
src/l2ps/channel/negotiate.ts
Defines RfqState, RfqProposalBody, RfqAcceptBody, RfqEndBody, StandingProposal, RfqOutcome, RfqSessionOpts, and internal RFQ_TYPES allowlist.
RfqSession class: storage, accessors, and outbound actions
src/l2ps/channel/negotiate.ts
Implements RfqSession with internal state fields, public accessors (state, standingProposal, outcome), outbound methods (offer, counter, accept, reject, abort), private proposeOutgoing helper, and settle/assertOpen helpers.
RfqSession.onIncoming: inbound message dispatch
src/l2ps/channel/negotiate.ts
Implements onIncoming filtering non-RFQ/echo/post-terminal messages and dispatching offer/counter (update standing, fire onProposal), accept (resolve from proposals map, throw on unknown sequence), and reject/abort (settle terminal outcomes).
Channel index re-exports and test suite
src/l2ps/channel/index.ts, src/tests/l2ps/negotiate.test.ts
Re-exports all RFQ public types from the channel index. Test suite uses a two-party in-memory harness to validate happy-path settlement, rejection propagation, accept reference resolution, illegal transition errors, terminal-state guards, unknown-sequence throwing, and opaque term propagation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

  • SR-4 / L2PS — Private Negotiation Channel for DACS-3 node#929: This PR implements the negotiate-rfq phase state machine (RfqSession) for SR-4, which is the direct continuation of the L2PS channel infrastructure work described in that issue, which explicitly called out negotiate-rfq as a target phase to enable end-to-end.

Poem

🐇 Hop, hop — a proposal lands in the field,
A counter arrives, and the terms are revealed.
Accept or reject, the state machine knows,
No echoes allowed where negotiation goes.
The standing proposal stands tall with grace,
Each sequence remembered, each outcome in place.
✨ The rabbit approves this turn-based embrace!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically identifies the main change: implementation of SR-4 WI-B negotiate-rfq phase logic, which aligns with the core addition of the RfqSession class and state machine for handling RFQ negotiations.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/sr4-wi-b-negotiate-rfq

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (5)
src/l2ps/channel/negotiate.ts (2)

110-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add JSDoc for outcome() to match new-method documentation rules.

Suggested change
+    /** Snapshot of the latest negotiated outcome/state. */
     outcome(): RfqOutcome {
         return this._outcome
     }

As per coding guidelines, **/*.{ts,tsx}: Use JSDoc format for all new methods and functions.

🤖 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/negotiate.ts` around lines 110 - 112, The outcome() method
in the negotiate.ts file is missing JSDoc documentation. Add JSDoc comment block
above the outcome() method that describes what the method does, its return type,
and any relevant details. Follow standard JSDoc format with description and
`@returns` tag to document that it returns an RfqOutcome value, ensuring
consistency with the codebase documentation standards.

Source: Coding guidelines


21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use project path aliases instead of relative imports.

These imports violate the repository TS import rule and make path movement harder.

Suggested change
-import type { ChannelMessage, ChannelMessageType } from "./types"
-import type { ClaimReference } from "../../identity/cci"
+import type { ChannelMessage, ChannelMessageType } from "`@/l2ps/channel/types`"
+import type { ClaimReference } from "`@/identity/cci`"

As per coding guidelines, **/*.{ts,tsx}: 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/channel/negotiate.ts` around lines 21 - 22, Replace the relative
imports with project path aliases using the `@/` prefix as required by the
repository's import guidelines. Specifically, change the import statement for
ClaimReference from the relative path `../../identity/cci` to use the
`@/identity/cci` alias, and similarly update the import from `./types` to use
`@/l2ps/channel/types` to maintain consistency with the project's path alias
conventions for all TypeScript imports.

Source: Coding guidelines

src/tests/l2ps/negotiate.test.ts (3)

1-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the channel public entrypoint in this suite to validate re-export contracts.

RfqSession and RFQ types are imported from @/l2ps/channel/negotiate, which bypasses the new public surface added in src/l2ps/channel/index.ts. Import these from @/l2ps/channel so this test fails if re-exports regress.

Suggested diff
-import {
-    RfqSession,
-    type RfqAcceptBody,
-    type RfqProposalBody,
-} from "`@/l2ps/channel/negotiate`"
-import type { ChannelMessage, ChannelMessageType } from "`@/l2ps/channel`"
+import {
+    RfqSession,
+    type RfqAcceptBody,
+    type RfqProposalBody,
+    type ChannelMessage,
+    type ChannelMessageType,
+} from "`@/l2ps/channel`"
🤖 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/tests/l2ps/negotiate.test.ts` around lines 1 - 6, The test suite is
importing RfqSession, RfqAcceptBody, and RfqProposalBody from the internal
module path "`@/l2ps/channel/negotiate`" instead of the public entrypoint. Change
the import statement to import these types from "`@/l2ps/channel`" instead, so the
test validates that the re-export contracts are properly maintained in the
public surface at src/l2ps/channel/index.ts.

79-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add an explicit abort terminal-state test.

reject is covered, but abort is another terminal transition in the same state machine. A symmetric test here would close an easy regression gap.

🤖 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/tests/l2ps/negotiate.test.ts` around lines 79 - 88, The test suite
currently covers the reject terminal state transition with the "reject
terminates both sides" test case, but lacks coverage for the abort terminal
state which is another terminal transition in the same state machine. Add a new
test case called "abort terminates both sides" that mirrors the structure of the
existing reject test but instead calls the abort method on one session and
verifies that both aSes.state and bSes.state reach the "aborted" terminal state,
similar to how the reject test checks for "rejected" state.

25-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reduce as ChannelMessage assertions to keep compile-time envelope checks intact.

The current casts suppress structural type validation in the harness and bogus-message fixture. Prefer typed objects (or satisfies) so schema drift is caught by TypeScript.

Suggested diff
-    const mkSend =
-        (me: string, peer: () => RfqSession) =>
+    const mkSend =
+        (me: ClaimReference, peer: () => RfqSession) =>
         async (opts: {
             type: ChannelMessageType
             body: unknown
             repliesTo?: number
         }): Promise<ChannelMessage> => {
             const msg = {
                 channelId: "ch",
                 sequence: ++seq,
                 sender: me,
                 sentAt: 1000 + seq,
                 type: opts.type,
                 body: opts.body,
                 ...(opts.repliesTo !== undefined && {
                     refs: { repliesTo: opts.repliesTo },
                 }),
                 signature: { sigVersion: "1", signature: "0xsig" + seq },
-            } as ChannelMessage
+            } satisfies ChannelMessage
             // Deliver to the counterparty after the local state settles.
             queueMicrotask(() => peer().onIncoming(msg))
             return msg
         }
@@
-        const bogus = {
+        const bogus: ChannelMessage = {
             channelId: "ch",
             sequence: 9,
             sender: B,
             sentAt: 9,
-            type: "accept" as ChannelMessageType,
-            body: { acceptedSequence: 42 } as RfqAcceptBody,
+            type: "accept",
+            body: { acceptedSequence: 42 } satisfies RfqAcceptBody,
             signature: { sigVersion: "1", signature: "0xsig9" },
-        } as ChannelMessage
+        }

As per coding guidelines, **/*.{ts,tsx} should ensure full TypeScript type coverage and run type checks after changes.

Also applies to: 131-140

🤖 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/tests/l2ps/negotiate.test.ts` around lines 25 - 43, The type assertion
`as ChannelMessage` on the msg object definition is suppressing TypeScript's
structural type validation, which prevents catching schema drift at compile
time. Replace the `as ChannelMessage` assertion with `satisfies ChannelMessage`
to maintain compile-time envelope checks while still allowing the object to be
properly typed. This change should be applied to the msg object construction
(and also at line 131-140 as mentioned in the comment) to ensure TypeScript
validates the structure without losing type safety.

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/l2ps/channel/negotiate.ts`:
- Around line 115-168: The methods offer, counter, accept, reject, and abort
perform state validation checks (assertOpen and _standing checks) before
awaiting the asynchronous sendFn call, allowing parallel calls to pass checks
independently and then cause conflicting state transitions. Add a serialization
mechanism such as a queue or promise-based lock to ensure only one outbound
action executes at a time, wrapping the entire operation (checks, sendFn call,
and settle) so that concurrent calls wait for the previous operation to complete
before proceeding.
- Around line 181-194: In the switch statement handling "offer" and "counter"
message types, add validation to enforce transition legality: for "counter"
messages, verify that a current standing proposal exists and that the counter is
properly linked to it (likely by checking a reference or sequence relationship
in the message body) before accepting it and updating the standing state. Only
proceed with setting this.proposals and this._standing if the counter transition
is valid; otherwise, reject or ignore the invalid counter message to prevent
out-of-turn proposals from replacing the current standing proposal.

---

Nitpick comments:
In `@src/l2ps/channel/negotiate.ts`:
- Around line 110-112: The outcome() method in the negotiate.ts file is missing
JSDoc documentation. Add JSDoc comment block above the outcome() method that
describes what the method does, its return type, and any relevant details.
Follow standard JSDoc format with description and `@returns` tag to document that
it returns an RfqOutcome value, ensuring consistency with the codebase
documentation standards.
- Around line 21-22: Replace the relative imports with project path aliases
using the `@/` prefix as required by the repository's import guidelines.
Specifically, change the import statement for ClaimReference from the relative
path `../../identity/cci` to use the `@/identity/cci` alias, and similarly
update the import from `./types` to use `@/l2ps/channel/types` to maintain
consistency with the project's path alias conventions for all TypeScript
imports.

In `@src/tests/l2ps/negotiate.test.ts`:
- Around line 1-6: The test suite is importing RfqSession, RfqAcceptBody, and
RfqProposalBody from the internal module path "`@/l2ps/channel/negotiate`" instead
of the public entrypoint. Change the import statement to import these types from
"`@/l2ps/channel`" instead, so the test validates that the re-export contracts are
properly maintained in the public surface at src/l2ps/channel/index.ts.
- Around line 79-88: The test suite currently covers the reject terminal state
transition with the "reject terminates both sides" test case, but lacks coverage
for the abort terminal state which is another terminal transition in the same
state machine. Add a new test case called "abort terminates both sides" that
mirrors the structure of the existing reject test but instead calls the abort
method on one session and verifies that both aSes.state and bSes.state reach the
"aborted" terminal state, similar to how the reject test checks for "rejected"
state.
- Around line 25-43: The type assertion `as ChannelMessage` on the msg object
definition is suppressing TypeScript's structural type validation, which
prevents catching schema drift at compile time. Replace the `as ChannelMessage`
assertion with `satisfies ChannelMessage` to maintain compile-time envelope
checks while still allowing the object to be properly typed. This change should
be applied to the msg object construction (and also at line 131-140 as mentioned
in the comment) to ensure TypeScript validates the structure without losing type
safety.
🪄 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: 007799e7-92da-468b-9a18-311e3f175005

📥 Commits

Reviewing files that changed from the base of the PR and between d842ee9 and bd7860b.

📒 Files selected for processing (3)
  • src/l2ps/channel/index.ts
  • src/l2ps/channel/negotiate.ts
  • src/tests/l2ps/negotiate.test.ts

Comment on lines +115 to +168
async offer(terms: unknown): Promise<ChannelMessage> {
this.assertOpen("offer")
if (this._standing)
throw new Error(
"RfqSession: a proposal already stands; use counter()",
)
return this.proposeOutgoing("offer", terms)
}

/** Counter the standing proposal with new terms. */
async counter(terms: unknown): Promise<ChannelMessage> {
this.assertOpen("counter")
if (!this._standing)
throw new Error("RfqSession: nothing to counter — no standing offer")
return this.proposeOutgoing("counter", terms, this._standing.sequence)
}

/** Accept the standing proposal — terminal (CH-5). */
async accept(): Promise<ChannelMessage> {
this.assertOpen("accept")
if (!this._standing)
throw new Error("RfqSession: nothing to accept — no standing offer")
const accepted = this._standing
const body: RfqAcceptBody = { acceptedSequence: accepted.sequence }
const msg = await this.sendFn({
type: "accept",
body,
repliesTo: accepted.sequence,
})
this.settle({
state: "accepted",
agreedTerms: accepted.terms,
acceptedSequence: accepted.sequence,
})
return msg
}

/** Reject the negotiation — terminal (CH-5). */
async reject(reason?: string): Promise<ChannelMessage> {
this.assertOpen("reject")
const body: RfqEndBody = reason ? { reason } : {}
const msg = await this.sendFn({ type: "reject", body })
this.settle({ state: "rejected", reason })
return msg
}

/** Abort the negotiation — terminal (CH-5). */
async abort(reason?: string): Promise<ChannelMessage> {
this.assertOpen("abort")
const body: RfqEndBody = reason ? { reason } : {}
const msg = await this.sendFn({ type: "abort", body })
this.settle({ state: "aborted", reason })
return msg
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Serialize outbound actions to avoid async state races.

assertOpen/standing checks run before await this.sendFn(...). Parallel calls can both pass checks and emit conflicting transitions/proposals.

Suggested change
 export class RfqSession {
+    private _actionInFlight = false
+
+    private async runOpenAction<T>(
+        action: string,
+        fn: () => Promise<T>,
+    ): Promise<T> {
+        if (this._actionInFlight) {
+            throw new Error(
+                `RfqSession: cannot ${action} — another action is in flight`,
+            )
+        }
+        this.assertOpen(action)
+        this._actionInFlight = true
+        try {
+            return await fn()
+        } finally {
+            this._actionInFlight = false
+        }
+    }
+
     /** Open a negotiation with the first proposal. */
     async offer(terms: unknown): Promise<ChannelMessage> {
-        this.assertOpen("offer")
-        if (this._standing)
-            throw new Error(
-                "RfqSession: a proposal already stands; use counter()",
-            )
-        return this.proposeOutgoing("offer", terms)
+        return this.runOpenAction("offer", async () => {
+            if (this._standing)
+                throw new Error(
+                    "RfqSession: a proposal already stands; use counter()",
+                )
+            return this.proposeOutgoing("offer", terms)
+        })
     }

Also applies to: 226-242

🤖 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/negotiate.ts` around lines 115 - 168, The methods offer,
counter, accept, reject, and abort perform state validation checks (assertOpen
and _standing checks) before awaiting the asynchronous sendFn call, allowing
parallel calls to pass checks independently and then cause conflicting state
transitions. Add a serialization mechanism such as a queue or promise-based lock
to ensure only one outbound action executes at a time, wrapping the entire
operation (checks, sendFn call, and settle) so that concurrent calls wait for
the previous operation to complete before proceeding.

Comment on lines +181 to +194
switch (msg.type) {
case "offer":
case "counter": {
const terms = (msg.body as RfqProposalBody)?.terms
const proposal: StandingProposal = {
sequence: msg.sequence,
sender: msg.sender,
terms,
}
this.proposals.set(msg.sequence, proposal)
this._standing = proposal
this.onProposal?.(proposal)
break
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Enforce inbound offer/counter transition legality.

Current handling accepts any inbound proposal and replaces standing state, even if counter is out-of-turn or not linked to the current standing proposal.

Suggested change
         switch (msg.type) {
-            case "offer":
-            case "counter": {
+            case "offer": {
+                if (this._standing) {
+                    throw new Error(
+                        "RfqSession: illegal offer — a proposal already stands; expected counter",
+                    )
+                }
+                const terms = (msg.body as RfqProposalBody)?.terms
+                const proposal: StandingProposal = {
+                    sequence: msg.sequence,
+                    sender: msg.sender,
+                    terms,
+                }
+                this.proposals.set(msg.sequence, proposal)
+                this._standing = proposal
+                this.onProposal?.(proposal)
+                break
+            }
+            case "counter": {
+                if (!this._standing) {
+                    throw new Error(
+                        "RfqSession: illegal counter — no standing proposal to counter",
+                    )
+                }
+                if (msg.refs?.repliesTo !== this._standing.sequence) {
+                    throw new Error(
+                        `RfqSession: illegal counter — repliesTo must target standing proposal sequence ${this._standing.sequence}`,
+                    )
+                }
                 const terms = (msg.body as RfqProposalBody)?.terms
                 const proposal: StandingProposal = {
                     sequence: msg.sequence,
                     sender: msg.sender,
                     terms,
                 }
                 this.proposals.set(msg.sequence, proposal)
                 this._standing = proposal
                 this.onProposal?.(proposal)
                 break
             }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
switch (msg.type) {
case "offer":
case "counter": {
const terms = (msg.body as RfqProposalBody)?.terms
const proposal: StandingProposal = {
sequence: msg.sequence,
sender: msg.sender,
terms,
}
this.proposals.set(msg.sequence, proposal)
this._standing = proposal
this.onProposal?.(proposal)
break
}
switch (msg.type) {
case "offer": {
if (this._standing) {
throw new Error(
"RfqSession: illegal offer — a proposal already stands; expected counter",
)
}
const terms = (msg.body as RfqProposalBody)?.terms
const proposal: StandingProposal = {
sequence: msg.sequence,
sender: msg.sender,
terms,
}
this.proposals.set(msg.sequence, proposal)
this._standing = proposal
this.onProposal?.(proposal)
break
}
case "counter": {
if (!this._standing) {
throw new Error(
"RfqSession: illegal counter — no standing proposal to counter",
)
}
if (msg.refs?.repliesTo !== this._standing.sequence) {
throw new Error(
`RfqSession: illegal counter — repliesTo must target standing proposal sequence ${this._standing.sequence}`,
)
}
const terms = (msg.body as RfqProposalBody)?.terms
const proposal: StandingProposal = {
sequence: msg.sequence,
sender: msg.sender,
terms,
}
this.proposals.set(msg.sequence, proposal)
this._standing = proposal
this.onProposal?.(proposal)
break
}
🤖 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/negotiate.ts` around lines 181 - 194, In the switch
statement handling "offer" and "counter" message types, add validation to
enforce transition legality: for "counter" messages, verify that a current
standing proposal exists and that the counter is properly linked to it (likely
by checking a reference or sequence relationship in the message body) before
accepting it and updating the standing state. Only proceed with setting
this.proposals and this._standing if the counter transition is valid; otherwise,
reject or ignore the invalid counter message to prevent out-of-turn proposals
from replacing the current standing proposal.

@greptile-apps

greptile-apps Bot commented Jun 23, 2026

Copy link
Copy Markdown

Greptile Summary

This PR introduces RfqSession (src/l2ps/channel/negotiate.ts), a pure protocol state machine that drives the negotiate-rfq phase of SR-4: offer/counter exchange terminating in accept, reject, or abort (CH-5). The session is transport-agnostic — callers wire send to L2PSChannelTransport.send and push verified inbound envelopes into onIncoming.

  • State machine tracks open | accepted | rejected | aborted, maintains a proposals map keyed by sequence, and resolves agreedTerms from the exact proposal sequence the accept points at — correctly handling multi-round counter chains.
  • Missing sender-identity guards in accept() and counter(): a party who just sent an offer can immediately call accept(), accepting its own proposal and forcing the remote side's onIncoming to settle with those terms — both sessions end up in accepted state without the counterparty's explicit consent.
  • Test coverage is solid for the normal flows but test 5 inadvertently relies on the self-accept path rather than asserting it is a protocol error, leaving the gap invisible.

Confidence Score: 3/5

The state machine core is sound, but a missing sender check in accept() lets one party unilaterally settle both sessions on their own terms — the counterparty is forced into accepted without ever consenting.

The self-accept path in accept() is a real protocol correctness defect: after calling offer(), a party can immediately call accept() and settle both RfqSessions with its own terms, bypassing the counterparty's consent entirely. The remote side's onIncoming locates the proposal in its map and calls settle() — the negotiation completes with no explicit agreement from the other party. Test 5 exercises this path without asserting it is an error, so the gap goes undetected by the suite.

src/l2ps/channel/negotiate.ts — the accept() and counter() methods need sender-identity guards; src/tests/l2ps/negotiate.test.ts — test 5 should assert that self-accept throws rather than relying on it to reach a terminal state.

Important Files Changed

Filename Overview
src/l2ps/channel/negotiate.ts New RfqSession state machine — core logic sound, but accept() and counter() lack sender-identity guards, allowing a party to accept or counter their own standing proposal and force both sides into a settled state.
src/l2ps/channel/index.ts Re-exports all public negotiate.ts types and RfqSession; additive, no regressions.
src/tests/l2ps/negotiate.test.ts Seven tests cover the main flows; test 5 unknowingly exercises the self-accept path without asserting that it is a protocol error, so the self-accept bug goes undetected.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant A as Party A (RfqSession)
    participant T as Transport
    participant B as Party B (RfqSession)

    Note over A,B: Normal flow
    A->>T: "offer(terms) — sendFn type:offer seq=1"
    T-->>A: "signed ChannelMessage seq=1"
    A->>A: "proposals[1]={seq:1,sender:A} _standing=proposals[1]"
    T->>B: "onIncoming(offer, seq=1)"
    B->>B: "proposals[1]={seq:1,sender:A} _standing=proposals[1]"
    B->>T: "counter(terms2) — sendFn type:counter seq=2"
    T-->>B: "signed ChannelMessage seq=2"
    B->>B: "proposals[2]={seq:2,sender:B} _standing=proposals[2]"
    T->>A: "onIncoming(counter, seq=2)"
    A->>A: "proposals[2]={seq:2,sender:B} _standing=proposals[2]"
    A->>T: "accept() — sendFn type:accept acceptedSequence=2"
    T-->>A: "signed ChannelMessage seq=3"
    A->>A: "settle(accepted, agreedTerms=terms2)"
    T->>B: "onIncoming(accept, acceptedSequence=2)"
    B->>B: proposals.get(2) found — settle(accepted)

    Note over A,B: Bug path — self-accept (no guard)
    A->>T: "offer(terms) seq=1 — _standing.sender===A"
    A->>T: "accept() — _standing.sender===A, no guard!"
    T->>B: "onIncoming(accept, acceptedSequence=1)"
    B->>B: proposals.get(1) found — settle(accepted) without B consenting
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant A as Party A (RfqSession)
    participant T as Transport
    participant B as Party B (RfqSession)

    Note over A,B: Normal flow
    A->>T: "offer(terms) — sendFn type:offer seq=1"
    T-->>A: "signed ChannelMessage seq=1"
    A->>A: "proposals[1]={seq:1,sender:A} _standing=proposals[1]"
    T->>B: "onIncoming(offer, seq=1)"
    B->>B: "proposals[1]={seq:1,sender:A} _standing=proposals[1]"
    B->>T: "counter(terms2) — sendFn type:counter seq=2"
    T-->>B: "signed ChannelMessage seq=2"
    B->>B: "proposals[2]={seq:2,sender:B} _standing=proposals[2]"
    T->>A: "onIncoming(counter, seq=2)"
    A->>A: "proposals[2]={seq:2,sender:B} _standing=proposals[2]"
    A->>T: "accept() — sendFn type:accept acceptedSequence=2"
    T-->>A: "signed ChannelMessage seq=3"
    A->>A: "settle(accepted, agreedTerms=terms2)"
    T->>B: "onIncoming(accept, acceptedSequence=2)"
    B->>B: proposals.get(2) found — settle(accepted)

    Note over A,B: Bug path — self-accept (no guard)
    A->>T: "offer(terms) seq=1 — _standing.sender===A"
    A->>T: "accept() — _standing.sender===A, no guard!"
    T->>B: "onIncoming(accept, acceptedSequence=1)"
    B->>B: proposals.get(1) found — settle(accepted) without B consenting
Loading

Reviews (1): Last reviewed commit: "feat(l2ps): SR-4 WI-B — negotiate-rfq ph..." | Re-trigger Greptile

Comment on lines +113 to +126

/** Open a negotiation with the first proposal. */
async offer(terms: unknown): Promise<ChannelMessage> {
this.assertOpen("offer")
if (this._standing)
throw new Error(
"RfqSession: a proposal already stands; use counter()",
)
return this.proposeOutgoing("offer", terms)
}

/** Counter the standing proposal with new terms. */
async counter(terms: unknown): Promise<ChannelMessage> {
this.assertOpen("counter")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Self-accept bypasses negotiation consent

accept() does not check that this._standing.sender !== this.me, so a party who just sent an offer or counter can immediately call accept() and unilaterally settle both sides. When B receives that accept message, its onIncoming finds the proposal in its map (added when B received the offer) and calls settle() — leaving B "accepted" without B ever explicitly agreeing. Test 5 ("refuses any action after terminal state") even exercises this path: A offers then immediately calls accept(), settling both sessions on A's own terms. A guard like if (accepted.sender === this.me) throw new Error("RfqSession: cannot accept own proposal") is needed to enforce the two-party consent invariant.

Comment on lines +103 to +110
get state(): RfqState {
return this._state
}
/** The proposal currently on the table, or null before the first offer. */
get standingProposal(): StandingProposal | null {
return this._standing
}
outcome(): RfqOutcome {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 counter() allows self-countering a standing own proposal

counter() only checks !this._standing but not whether this._standing.sender === this.me. After A calls offer(), _standing is set to A's own proposal. A can immediately call counter() to replace it with a new proposal, effectively spamming updates before the counterparty has responded. Like the self-accept issue, the echo guard in onIncoming (msg.sender === this.me) shows the design intent was to distinguish own vs. counterparty messages — counter() should apply the same check.

Comment on lines +84 to +89
export class RfqSession {
private readonly me: ClaimReference
private readonly sendFn: RfqSessionOpts["send"]
private readonly onStateChange?: (o: RfqOutcome) => void
private readonly onProposal?: (p: StandingProposal) => void

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 proposals map grows without bound

Every offer and counter is inserted into this.proposals but entries are never removed. In a long counter-chain, the map retains every historical proposal for the lifetime of the session. Since the only lookup is the final acceptedSequence in onIncoming, completed rounds are dead entries. Consider clearing superseded entries in settle(), or at minimum documenting that callers should keep negotiation rounds short.

@sonarqubecloud

Copy link
Copy Markdown

@tcsenpai

Copy link
Copy Markdown
Contributor

Auto-closed by the merge of #99 (combined WI-A/B/C, merged as 2a6baa3). All Greptile findings from this slice were addressed there (5/5).

@tcsenpai tcsenpai deleted the feat/sr4-wi-b-negotiate-rfq branch June 24, 2026 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants