feat(x402-v2): fix dual-mode dead-code bug via Option C refactor (Story 14)#36
Merged
Merged
Conversation
…g dual-mode (Story 14) Bug: pre-existing if/else-if chain in `signPayment` (src/x402-v2.ts line 251+) included an unreachable `else if (protocol === 'dual')` arm — line 163 already matched `dual` because of `protocol === 'x402' || protocol === 'dual'`. The dead branch was meant to produce both an x402 EIP-3009 signature AND an MPP PaymentAuthorization signature; instead, dual-mode silently fell into the x402-only branch and attached MPP fields as inert metadata under `signedData.mppFormat` — no real MPP signature was ever produced. Bug origin commit: dbe4c89 (2026-05-20 rebase artifact). Customer impact: ZERO at present (paybot-sdk 0.3.0 unreleased; paybot-mcp and paybot-core pin to ^0.2.0). Fix lands before 0.3.0 ships. Fix (Option C per 2026-05-22 roundtable verdict): - Extracted `signX402(account, requirements)` private helper, verbatim body from the pre-refactor x402 branch. - Extracted `signMPP(account, requirements, intentId)` private helper, verbatim body from the pre-refactor mpp branch. - Rewrote `signPayment` as a `switch` dispatcher with explicit `x402` / `mpp` / `dual` cases and a `default` arm throwing PayBotApiError with code `UNSUPPORTED_PROTOCOL` (status 402). - Dual-mode now calls BOTH helpers; `signedData = { x402, mpp }` packs both signed payloads; top-level `signature` mirrors the x402 signature for legacy compatibility. - Deleted the unreachable dead-code branch (was lines 251-311). No silent behavior change for `x402` or `mpp` consumers — proven by Test 13 (byte-for-byte regression). Tests: new tests/x402-v2.test.ts (was zero) — 13 unit tests covering signX402 happy/error/edge, signMPP happy/error/edge, dispatcher x402/mpp/ dual/unsupported/missing-key paths, and two regression tests including the canonical Test 12 that runs EIP-712 signature recovery against the dual-mode MPP signature and confirms it recovers the signer address (i.e., proves the signature is real cryptography, not metadata). Determinism: vi.useFakeTimers + vi.setSystemTime + mocked generateEIP3009Nonce. Quality: 115/115 tests pass (102 existing + 13 new); lint clean; typecheck clean; all new code lines (signX402 / signMPP / signPayment) hit. JSDoc added to all three methods with @param / @returns / @throws / @example. Out of scope per Story 14: discriminated union for signedData, broader coverage backfill on other untested public methods (Task #15), 0.3.0 release. SignedPayment.signedData stays Record<string, unknown>; no breaking type change. Refs: docs/stories/14.dual-mode-refactor.md (AIOX-Enterprise) SDC chain: @sm draft → @po validated 12/12 → @dev (this PR) → @qa pending → @devops merge. 🤖 Generated with Claude Code Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
![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 refactors the signPayment method in X402Handler by extracting protocol-specific logic into signX402 and signMPP private methods and implementing a switch dispatcher. This change ensures that the dual protocol mode correctly generates both x402 and MPP signatures. Additionally, a new test suite is introduced to provide regression testing and verify protocol support. Review feedback identified a critical bug in the signMPP implementation where the expiration timestamp and payment intent ID in the metadata did not match the values used for the signature, which would lead to verification failures.
Comment on lines
+247
to
+281
| const nonce = generateEIP3009Nonce(); | ||
| const nowSeconds = BigInt(Math.floor(Date.now() / 1000)); | ||
|
|
||
| const signature = await account.signTypedData({ | ||
| domain, | ||
| types: { | ||
| PaymentAuthorization: [ | ||
| { name: 'payer', type: 'address' }, | ||
| { name: 'recipient', type: 'address' }, | ||
| { name: 'amount', type: 'uint256' }, | ||
| { name: 'nonce', type: 'bytes32' }, | ||
| { name: 'expires', type: 'uint256' }, | ||
| { name: 'paymentIntent', type: 'string' }, | ||
| ], | ||
| }, | ||
| primaryType: 'PaymentAuthorization', | ||
| message: { | ||
| payer: account.address, | ||
| recipient: requirements.payTo as `0x${string}`, | ||
| amount: BigInt(requirements.amount), | ||
| nonce, | ||
| expires: nowSeconds + BigInt(3600), | ||
| paymentIntent: intentId || 'unknown', | ||
| }, | ||
| }); | ||
|
|
||
| const signedData: Record<string, unknown> = { | ||
| payer: account.address, | ||
| recipient: requirements.payTo, | ||
| amount: requirements.amount, | ||
| nonce, | ||
| expires: nowSeconds.toString(), | ||
| paymentIntent: intentId, | ||
| signature, | ||
| }; |
There was a problem hiding this comment.
The signMPP method contains logic discrepancies between the signed message and the returned signedData, which will cause verification failures for recipients.
- Expiration Mismatch: The signature is generated using
nowSeconds + BigInt(3600), butsignedData.expiresis set tonowSeconds.toString(). This makes the payload appear expired immediately to any verifier, as the signed expiration time is 1 hour in the future while the metadata claims it expires now. - Intent ID Mismatch: The signature uses
intentId || 'unknown', butsignedData.paymentIntentuses the rawintentId. IfintentIdis falsy (e.g.,undefinedor''), the verifier will receive a value that doesn't match the one used for the signature recovery.
Extracting these to local variables ensures consistency between the cryptographic signature and the returned metadata.
const nonce = generateEIP3009Nonce();
const nowSeconds = BigInt(Math.floor(Date.now() / 1000));
const expires = nowSeconds + BigInt(3600);
const paymentIntent = intentId || 'unknown';
const signature = await account.signTypedData({
domain,
types: {
PaymentAuthorization: [
{ name: 'payer', type: 'address' },
{ name: 'recipient', type: 'address' },
{ name: 'amount', type: 'uint256' },
{ name: 'nonce', type: 'bytes32' },
{ name: 'expires', type: 'uint256' },
{ name: 'paymentIntent', type: 'string' },
],
},
primaryType: 'PaymentAuthorization',
message: {
payer: account.address,
recipient: requirements.payTo as `0x${string}`,
amount: BigInt(requirements.amount),
nonce,
expires,
paymentIntent,
},
});
const signedData: Record<string, unknown> = {
payer: account.address,
recipient: requirements.payTo,
amount: requirements.amount,
nonce,
expires: expires.toString(),
paymentIntent,
signature,
};
Owner
Author
|
Merge authorization per .claude/rules/automated-pr-merge-authority.md (6th application — FIRST story-driven PR of the day):
Closes Task #14. Fixes dual-mode dead-code bug discovered 2026-05-22:
Non-blocking observations from @qa:
Precedent chain (today, 2026-05-22):
This is the 8th merged PR in the paybot ecosystem in a single day. |
RBKunnela
added a commit
that referenced
this pull request
May 22, 2026
Add `npm run lint` as a CI step in the build matrix job, positioned between `npm ci` (install) and `npm run type-check` so lint failures fast-fail before downstream type-check/test/coverage/build steps. Now possible because Task #14 (dual-mode dead-code fix in PR #36) eliminated the no-dupe-else-if blocker on `src/x402-v2.ts:251`. Lint passes clean on main (verified locally pre-commit). Why lint gate matters: paybot-sdk auto-publishes to npm on main push. Without a CI lint gate, lint regressions could ship to the registry. Note on Task #16 (coverage/ in .gitignore): already present on line 12 of `.gitignore`. No-op; not included in this PR. Closes #17 Refs #16 (no-op, pre-existing)
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.
Summary
Story 14 — Option C refactor. Fixes the dead-code bug in
X402Handler.signPaymentwhere the
else if (protocol === 'dual')arm (introduced by rebase artifactdbe4c89on 2026-05-20) was unreachable because the precedingif (protocol === 'x402' || protocol === 'dual')already matcheddual.Net effect of the bug: dual-mode silently fell through to x402-only signing
and attached MPP fields as inert metadata — no real MPP signature.
Roundtable verdict (2026-05-22): 3 specialists (@architect, @dev,
@qa-equivalent) agreed FIX over rebuild. Operator selected Option C
(extract-and-dispatch).
Customer impact: ZERO at present. paybot-sdk 0.3.0 is unreleased;
paybot-mcp + paybot-core pin to
^0.2.0. Fix lands before 0.3.0 ships.What changed
signX402(account, requirements)private helper — verbatim bodyfrom the pre-refactor x402 branch (current lines 165-205).
signMPP(account, requirements, intentId)private helper —verbatim body from the pre-refactor mpp branch (current lines 208-250).
signPaymentas aswitchdispatcher:x402→signX402onlympp→signMPPonlydual→ BOTHsignX402andsignMPP;signedData = { x402, mpp };top-level
signatureis the x402 signature (primary, legacy-compatible)default→ throwsPayBotApiErrorwith codeUNSUPPORTED_PROTOCOL(402)// WHYcomment on thedualcase explaining the design choice.Acceptance criteria (8/8 met with tests)
signX402byte-for-byte equivalentsignMPPbyte-for-byte equivalentsignPayment(x402)dispatch'x402'signPayment(mpp)dispatch'mpp'signPayment(dual)calls both, packs{ x402, mpp }'dual'defaultsignedData: Record<string, unknown>)src/types.tschangeTests added (13 new in
tests/x402-v2.test.ts)tests/x402-v2.test.tsis NEW — zero tests existed for the x402-v2 module.[UNIT] signX402 — should produce a verifiable EIP-3009 TransferWithAuthorization signature with typical inputs[UNIT] signX402 — should throw UNSUPPORTED_NETWORK PayBotApiError when network has no EIP-712 domain[UNIT] signX402 — should handle max-uint256 amount without overflow and produce a valid signature[UNIT] signMPP — should produce a valid PaymentAuthorization signature with typical inputs and intentId[UNIT] signMPP — should handle missing intentId by defaulting paymentIntent field to 'unknown'[UNIT] signMPP — should produce a different signature than signX402 for identical inputs (proves it uses different typed-data)[UNIT] signPayment — should dispatch to signX402 only when protocol is 'x402' and return identical shape to legacy behavior[UNIT] signPayment — should dispatch to signMPP only when protocol is 'mpp' and return identical shape to legacy behavior[UNIT] signPayment — should call BOTH signX402 and signMPP when protocol is 'dual' and pack signedData as { x402, mpp } with x402 signature as primary[UNIT] signPayment — should throw PayBotApiError with code UNSUPPORTED_PROTOCOL and status 402 for unknown protocol values[UNIT] signPayment — should throw PayBotApiError with code MISSING_WALLET_KEY when walletPrivateKey is not configured[UNIT] signPayment — dual-mode mpp signature should be a real cryptographic signature, not metadata (proves dead-code bug is fixed)— CANONICAL REGRESSION TEST for the original bug. RunsrecoverTypedDataAddressagainstsignedData.mpp.signaturewith the MPP typed-data structure and asserts the recovered address matches the signer. If the signature were inert metadata, recovery would fail or yield a wrong address.[UNIT] signPayment — dual-mode x402 signature byte-for-byte equals x402-only signature for the same inputs (proves refactor is non-breaking)— verbatim-migration regression shield.Determinism:
vi.useFakeTimers()+vi.setSystemTime('2026-05-22T12:00:00Z')+mocked
generateEIP3009Noncereturns fixed bytes32. Signatures byte-identicalacross runs.
Verbatim migration verified
Test 13 asserts
dual.signedData.x402istoEqualto the standalonex402-onlysignedDataobject, and thatdual.signatureequalsx402Only.signature. Same inputs (same fixed nonce, same fixed timestamp,same private key) produce byte-for-byte identical x402 signatures regardless
of whether the
signX402helper was reached via thex402case or thedualcase.Risk
Medium — crypto-signature code. Mitigations:
Quality gates (local — CI will rerun)
npm test— 115/115 PASS (102 existing + 13 new)npm run lint— PASS (clean)npm run type-check— PASS (clean)signX402/signMPP/signPayment): all lines hitglobal 80% threshold failure is pre-existing and out of scope per
Story 14 — Task chore(deps): bump vite, @vitest/coverage-v8 and vitest #15 covers project-wide coverage backfill)
Out of scope (Story 14 explicit exclusions)
signedData(would be a breaking type change)Chain governance
Full SINKRA chain per
automated-pr-merge-authority.md:Do not auto-merge. Per
automated-pr-merge-authority.mdonly @devopsclicks the merge button, and only after the full chain signs off.
References
D:\1.GITHUB\AIOX-Enterprise\docs\stories\14.dual-mode-refactor.mddbe4c89(2026-05-20).claude/rules/quality-foundation.md🤖 Generated with Claude Code