Skip to content

Protocol-spec drift — 5 patches surfaced by sphere-sdk#474 G4 audit #22

Description

@vrogojin

A drift audit of docs/protocol-spec.md v0.1 (Draft, 2026-04-03) against src/trader/*.ts was conducted as part of sphere-sdk issue #474 G4. The full audit (21 findings, ~530 lines) lives in unicity-sphere/sphere-sdk at docs/uxf/PROTOCOL-SPEC-DRIFT-474.md on PR #475.

This umbrella issue tracks the 5 spec patches the audit recommends. Each is independently shippable as a separate PR against docs/protocol-spec.md. They are all spec patches — the implementation is already what the spec should describe.


Patch 1: Rate / volume types are bigint strings, not floats (drift finding D1)

  • :139-142, :191-195, :399-402, :694-705, :1341 all declare rate_min/rate_max/volume_min/volume_max as number (JS float).
  • The implementation at src/trader/acp-types.ts:20-23 declares them as string (bigint-encoded); trader-command-handler.ts:370-378 parses via safeParseBigint; src/trader/types.ts:72-75 carries the canonical domain shape as bigint. safeParseBigint rejects any non-/^-?\\d+$/ literal — float-shaped strings like \"0.5\" are rejected as INVALID_PARAM at the ACP boundary.
  • Patch §2.4, §4.2, §4.4, §4.5 TypeScript interface blocks to use string (with doc comment // stringified bigint, smallest units).
  • Patch the §2.4 constraint table at :191-195 to read "Positive bigint (string-encoded, smallest units)".
  • Patch §5.2's floor((overlap_min + overlap_max) / 2) to clarify it's bigint integer division — implementation in intent-engine.ts:896 does Number((rateMin + rateMax) / 2n) for the MarketModule midpoint, but on the wire stays bigint.
  • Patch §7.3 assertions to use bigint comparison operators (> 0n etc.).

Patch 2: NP envelope signature input formula (drift finding D2)

  • :469 declares signature: string; // ECDSA over sha256hex(deal_id + \":\" + msg_id + \":\" + type).
  • Implementation at negotiation-handler.ts:561-563 and :815-823 actually signs sha256hex(canonicalJson(envelope-minus-signature)) — covers EVERY field, not just three.
  • Security-relevant: the spec formula lets a MITM tamper with payload (e.g. proposer_swap_address) while keeping the signature valid. The implementation's choice is correct.
  • Patch §3.4 line 469 to // ECDSA over sha256hex(canonicalJson(envelope-minus-signature)). Add a "Rationale: binds every field, prevents payload-substitution" sentence.

Patch 3: Deal ID derivation + DealTerms structure omit 4 fields (drift findings D3 + D4)

  • §3.5 (:487-500) hashes 10 fields. §3.6 (:505-518, 626-638) interface has 11 fields.
  • negotiation-handler.ts:531-551 computeDealId hashes 14 fields — adds proposer_address, acceptor_address, deposit_timeout_sec, proposer_direction.
  • types.ts:98-114 DealTerms carries 16 fields — adds deal_id itself plus the above three.
  • Security-relevant: without proposer_direction in the hash, an attacker could swap who-sells-what and the deal_id would be unchanged. Without addresses in the hash, who-deposits-to-where is unbound.
  • Patch §3.5 to add the four missing fields to the canonical JSON input.
  • Patch §3.6 to add deal_id, proposer_address, acceptor_address, proposer_direction to the interface.

Patch 4: ACP error code names (drift finding D5)

The spec publishes a closed set of error codes in §4 and Appendix B.1. The implementation returns different names:

  • :754-757 INTENT_NOT_FOUND → code returns NOT_FOUND (trader-command-handler.ts:479).
  • :724-728 MAX_INTENTS_REACHED → code returns LIMIT_EXCEEDED (:434).
  • :958-962 INVALID_ADDRESS → code returns INVALID_PARAM (:716).
  • :958-962 TRANSFER_FAILED → code returns WITHDRAW_FAILED (:761).
  • Appendix B.1 line 1604 lists WITHDRAWAL_LOCKED — never returned.
  • Generic INTERNAL_ERROR is returned at :462, 511, 540, 569, 624, 686, 868 but never listed in Appendix B.1.

Recommend renaming the spec to match the deployed implementation. Operators are already observing these names in the wild.

Patch 5: DEAL_REJECT_REASONS additions (drift finding D14)

  • §3.7 declares an 8-element enum DEAL_REJECT_REASONS.
  • Code at negotiation-handler.ts emits the 8 spec values PLUS 5 more: UNKNOWN_INTENT, ACCEPT_DM_SEND_FAILED, plus terminal-state mirrors that don't fit the original enum.
  • Patch §3.7 to add the 5 additional reason codes with one-line documentation each.

Out-of-scope

  • The CLI float UX work — tracked against unicity-sphere/sphere-cli.
  • The trader-agent v0.2 image rebuild — tracked against vrogojin/agentic_hosting.
  • Implementation changes (this issue is spec-side only).

Cross-references

  • The full audit document (21 findings, all line-cited) is at unicity-sphere/sphere-sdk/blob/feat/474-trader-roundtrip-soak/docs/uxf/PROTOCOL-SPEC-DRIFT-474.md (PR #475).
  • The non-finding section of the audit verifies that §3.3 NP message types, §3.4 envelope shape/size/dangerous-keys, §5.1 all 8 matching criteria, §5.7 proposer election, §7.4 expiry sweep, §7.6 dedup window, §7.7 limits all match implementation — these are explicitly OK.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions