Add signed receipt primitives#8
Conversation
|
Caution Review failedFailed to post review comments WalkthroughAdds an opt-in signed receipt system: new receipt types, deterministic canonicalization, EIP‑191 sign/verify primitives, client.pay wiring to request/return receipts, SDK re-exports, README docs, and tests validating signing and tamper detection. ChangesSigned Receipts Feature
Sequence DiagramsequenceDiagram
participant Client
participant signReceipt
participant canonicalize
participant viem as viem(privateKeyToAccount / signMessage)
Client->>signReceipt: UnsignedReceipt + privateKey
signReceipt->>viem: derive account (privateKeyToAccount)
signReceipt->>canonicalize: receiptSigningPayload(unsignedReceipt)
canonicalize->>canonicalize: normalize and stringify (sorted keys, omit undefined)
canonicalize-->>signReceipt: canonical payload
signReceipt->>viem: signMessage(EIP-191, payload)
viem-->>signReceipt: signature (0x...)
signReceipt-->>Client: SignedReceipt (signerAddress + signature)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
tests/client.test.tsParsing error: ESLint was configured to run on
tests/receipts.test.tsParsing error: ESLint was configured to run on
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/client.ts (1)
289-291: ⚡ Quick winAvoid blind-casting facilitator payloads to
SignedReceipt.This is an untrusted response boundary;
as SignedReceiptcan surface malformed data as a trusted SDK type. Add a minimal runtime guard before assigningsignedReceipt.As per coding guidelines,
src/**/*.tsshould preserve type safety and robust facilitator-failure handling.🤖 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/client.ts` around lines 289 - 291, The code blindly casts settleData.signedReceipt to SignedReceipt; add a minimal runtime type guard and fallback to avoid trusting unvalidated facilitator payloads. Implement an isSignedReceipt(value: any): value is SignedReceipt helper that checks the presence and basic types of the SignedReceipt required fields, then set signedReceipt using: signedReceipt: isSignedReceipt(settleData.signedReceipt) ? settleData.signedReceipt : undefined, and optionally log a warning when the guard fails; reference the settleData.signedReceipt property and the SignedReceipt type when making the change.
🤖 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/receipts.ts`:
- Line 22: The sort comparator inside normalize()/canonicalize() currently uses
localeCompare which is locale-sensitive; change the comparator used on
Object.entries(...).sort(...) (the line that currently calls a.localeCompare(b))
to perform a code-point (binary) string comparison instead by comparing a and b
with string relational operators and returning -1, 0, or 1 accordingly so
ordering is deterministic across environments.
In `@src/types.ts`:
- Around line 95-96: The ReceiptSignerRole union currently allows 'both' which
is unprovable with the single-signature receipt schema; change the type
definition (ReceiptSignerRole) to remove 'both' (leave only 'facilitator' |
'payer'), and update any codepaths that set or check signedBy to handle only
'facilitator' or 'payer' (or explicitly require separate signatures) so receipts
always carry a single signature and signerAddress; also add a short TODO comment
near ReceiptSignerRole referencing multi-signature support for future extension.
---
Nitpick comments:
In `@src/client.ts`:
- Around line 289-291: The code blindly casts settleData.signedReceipt to
SignedReceipt; add a minimal runtime type guard and fallback to avoid trusting
unvalidated facilitator payloads. Implement an isSignedReceipt(value: any):
value is SignedReceipt helper that checks the presence and basic types of the
SignedReceipt required fields, then set signedReceipt using: signedReceipt:
isSignedReceipt(settleData.signedReceipt) ? settleData.signedReceipt :
undefined, and optionally log a warning when the guard fails; reference the
settleData.signedReceipt property and the SignedReceipt type when making the
change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d909d26-f909-4eca-b8fd-f77bf756ae29
📒 Files selected for processing (7)
README.mdsrc/client.tssrc/index.tssrc/receipts.tssrc/types.tstests/client.test.tstests/receipts.test.ts
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces signed receipts to the SDK, enabling cryptographic proof of payment settlement bound to specific capabilities and artifacts. The changes include a new receipts.ts utility for EIP-191 signing and canonicalization, updated PaymentResult types to include receipts, and comprehensive test coverage. Review feedback highlights a potential non-determinism issue in the canonicalize function due to localeCompare and suggests strengthening type safety by making signerAddress required in the SignedReceipt interface.
| } | ||
| if (typeof value === 'object') { | ||
| const result: { [key: string]: JsonValue } = {}; | ||
| for (const [key, item] of Object.entries(value as Record<string, unknown>).sort(([a], [b]) => a.localeCompare(b))) { |
There was a problem hiding this comment.
Using localeCompare for sorting keys in a canonicalization function is risky because its behavior is environment-dependent (based on the system's default locale). For a deterministic canonical JSON representation that must be reproducible across different browsers and servers, a simple lexicographical comparison of UTF-16 code units is preferred.
| for (const [key, item] of Object.entries(value as Record<string, unknown>).sort(([a], [b]) => a.localeCompare(b))) { | |
| for (const [key, item] of Object.entries(value as Record<string, unknown>).sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0))) { |
| export interface SignedReceipt extends UnsignedReceipt { | ||
| /** EIP-191 signature over the canonical receipt payload */ | ||
| signature: `0x${string}`; | ||
| } |
There was a problem hiding this comment.
The SignedReceipt interface should ideally make signerAddress required, as a signed receipt is incomplete without the address of the party that generated the signature. This also improves type safety in the verifyReceipt function.
export interface SignedReceipt extends UnsignedReceipt {
/** The address that signed the receipt */
signerAddress: string;
/** EIP-191 signature over the canonical receipt payload */
signature: `0x${string}`;
}
|
Addressed the review notes in follow-up commits:
Validation after the changes: npm test
npm run type-check
npm run build
git diff --check |
|
Addressed the remaining response-boundary review note in
Validation after this change: npm test
npm run type-check
npm run build
npm run lint
git diff --check |
|
@TateLyman — thanks for shipping on Issue #7. Substantial work, fast turnaround. Per the project's standing contributor protocol, this PR goes through 4-agent vetting ( Two findings already surfaced by CodeRabbit + Gemini that are worth addressing in this PR rather than a follow-up, since both touch cryptographic correctness:
Both are CodeRabbit MAJOR findings, not nits. Architectural review starting now. CONTRIBUTORS.md addition lands with the merge per the Founding 10 program (#4). |
|
@TateLyman — thanks for the implementation work. 4-agent vetting (@devops → @architect → @dev → @qa) is complete. Synthesis: CONCERNS — not blocking on the foundation, but blocking on specific items below. Merge can proceed once these are addressed. 🤖 First, an open questionBranch name Vetting Results@devops (security / infrastructure) — CONCERNS
@architect (spec alignment) — CONCERNS
@dev (implementation + tests) — CONCERNS
@qa (acceptance criteria + regression) — CONCERNS7/8 acceptance criteria from Issue #7 met. Deviation:
Test coverage gaps (recommended, not all blocking):
Summary — what unblocks mergeRequired (4):
Required tests (2): Recommended (non-blocking):
Once these land, ping me — I'll re-vet (likely 30-min turnaround) and merge. |
|
Addressed the merge blockers in Required items:
Tests added/expanded:
Other cleanup:
Validation: npm test
npm run type-check
npm run build
npm run lint
git diff --checkReady for re-vet and CI approval/run. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/client.test.ts (1)
294-587: ⚡ Quick winExpand
pay()edge-case coverage for timeout, 402 retry, and insufficient balance.This suite is strong on happy-path and generic failures, but these three payment-flow edges are still untested in this file.
As per coding guidelines
tests/**: Review tests for: "Coverage of payment flow edge cases (402 retry, balance insufficient, timeout)".🤖 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 `@tests/client.test.ts` around lines 294 - 587, Add three new tests inside the existing describe('pay()') block to cover the missing edges: (1) "timeout" — mockFetch to reject with a timeout (e.g., new Error('ETIMEDOUT')) on both attempts and assert client.pay returns success: false and error contains 'ETIMEDOUT'; (2) "402 retry" — mockFetch first returns a 402 response (jsonResponse({ error: 'Payment required', code: 'PAYMENT_REQUIRED' }, 402)) and then a successful settle response on retry, assert that client.pay eventually returns success: true and that mockFetch was called twice; (3) "insufficient balance" — mockFetch verify returns valid with settlementToken but settle returns jsonResponse({ error: 'Insufficient balance', code: 'INSUFFICIENT_FUNDS' }, 402) and assert client.pay returns success: false and errorCode is 'INSUFFICIENT_FUNDS'. Use the same mockFetch and client.pay usage pattern and check call bodies via mockFetch.mock.calls like the surrounding tests.
🤖 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 `@README.md`:
- Around line 17-19: Add a language hint to the code fence containing
"PayBotClient → Facilitator (x402) → On-chain USDC (EIP-3009)" so the fence
begins with ```text (or another appropriate language) instead of just ```, which
will satisfy MD040; update the opening backticks for that snippet in README.md
to include the language tag.
In `@tests/client.test.ts`:
- Around line 356-361: Add a fourth malformed-case to the parameterized test
that verifies missing signerAddress is ignored: update the it.each invocation
used by the test named 'should ignore malformed signed receipts from settle
response: %s' to include an entry like ['missing signerAddress',
validSignedReceipt({ signerAddress: undefined })] (or validSignedReceipt({
signerAddress: '' }) if undefined isn't supported) so the negative matrix covers
the required signerAddress field; ensure you reference the existing
validSignedReceipt helper to construct the malformed receipt.
---
Nitpick comments:
In `@tests/client.test.ts`:
- Around line 294-587: Add three new tests inside the existing describe('pay()')
block to cover the missing edges: (1) "timeout" — mockFetch to reject with a
timeout (e.g., new Error('ETIMEDOUT')) on both attempts and assert client.pay
returns success: false and error contains 'ETIMEDOUT'; (2) "402 retry" —
mockFetch first returns a 402 response (jsonResponse({ error: 'Payment
required', code: 'PAYMENT_REQUIRED' }, 402)) and then a successful settle
response on retry, assert that client.pay eventually returns success: true and
that mockFetch was called twice; (3) "insufficient balance" — mockFetch verify
returns valid with settlementToken but settle returns jsonResponse({ error:
'Insufficient balance', code: 'INSUFFICIENT_FUNDS' }, 402) and assert client.pay
returns success: false and errorCode is 'INSUFFICIENT_FUNDS'. Use the same
mockFetch and client.pay usage pattern and check call bodies via
mockFetch.mock.calls like the surrounding tests.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a4ffba0-2557-4656-9dd7-b6902ae7494e
📒 Files selected for processing (6)
README.mdsrc/client.tssrc/receipts.tssrc/types.tstests/client.test.tstests/receipts.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client.ts
- src/receipts.ts
- src/types.ts
|
Pushed Changes:
One clarification: I kept the verify-time 402 behavior as no-retry for Validation after this commit: npm test
npm run type-check
npm run build
npm run lint
git diff --check |
RBKunnela
left a comment
RBKunnela
left a comment
There was a problem hiding this comment.
LGTM — ready to merge.
What's in: canonical JSON (sorted keys + undefined-dropped), EIP-191 signing via viem, signedBy: 'facilitator' | 'payer' role, optional artifact + reputation pointers, runtime guard before exposing settleData.signedReceipt. The negative matrix in tests/receipts.test.ts covers version mismatch, missing signerAddress, missing payer botId, missing settlement fields, and signature without 0x prefix. CodeRabbit's last actionable note (the signerAddress malformed case in tests/client.test.ts) is marked addressed in a0d18a1.
Outstanding nitpick — follow-up, not a blocker: the pay() edge-case coverage CodeRabbit asked for (timeout / 402 retry / insufficient balance) is genuinely missing in tests/client.test.ts but doesn't belong in the receipt PR. I'll open a separate issue for it.
Implementation model: keeping the hybrid arrangement — you own the account and day-to-day code; signature/treasury actions go through me as principal. Founding contributor entry going into CONTRIBUTORS.md with this merge.
Thank you for the fast turnaround on every review round. Merging shortly.
|
Merge-state note: the PR still shows mergeStateStatus=BLOCKED even though CodeRabbit latest status context is passing and your owner review is approved. The blocker appears to be the stale CodeRabbit CHANGES_REQUESTED review decision on an earlier commit, not CI. If branch protection treats bot review state as required, this may need the stale CodeRabbit review dismissed or a maintainer override. No code change from my side unless you want another commit. |
TateLyman
force-pushed
the
codex/signed-receipts
branch
from
a0d18a1 to
dbe4c89
Compare
|
Rebased onto current main and pushed |
TateLyman
force-pushed
the
codex/signed-receipts
branch
from
dbe4c89 to
f12ad8c
Compare
|
Rebased onto current Conflict resolved in Local validation after the rebase: npm test # 119 passed
npm run type-check
npm run lint
npm run build
git diff --checkCI/CodeRabbit is running on the refreshed branch now. |
2 participants
Summary
emitReceipt: truebefore requesting or returningPaymentResult.signedReceiptReceiptAgentfromAgentIdentityso receipt identity does not drift from the shared SDK identity typesignerAddress, payer signer-address matching, and the client receipt lookup helpers deferred to a follow-up PRRelated to #7. This PR covers receipt primitives plus opt-in
client.pay()receipt emission; lookup methods and multi-signature receipts remain follow-up work so #7 should stay open after merge.Verification
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests