fix(micropayment-engine): correct EIP-55 checksum on verifyingContract (Story 15.1)#37
Conversation
…ory 15.1)
Two related blockers preventing the signing path from working with
viem 2.49+ strict validation:
1. verifyingContract literal had wrong EIP-55 checksum (4 letter case-swaps).
Canonical address (verified via viem getAddress()):
0x50b0f7224fFC5F4F7685DbcE1B8b7e7B8b8A4A23
2. generateNonce() returned un-prefixed 64-char hex but the call site cast
it as `0x${string}` — a type-system lie. viem rejected at hashTypedData
with BytesSizeMismatchError ("expected bytes32, got bytes64"). Fixed
at the source: nonce is now genuinely 0x-prefixed and the return type
is honest.
Verified locally with an ad-hoc smoke script (not committed) — signTypedData
returns a 65-byte (132-char hex) signature and a 32-byte (66-char hex)
nonce after these two fixes. Test file in Story 15 (tests/micropayment-engine.test.ts)
will exercise this path properly post-merge.
Audit pass (Task 15.1.3) found no other hardcoded addresses in this file
and the remaining `as` casts on hex types are pre-existing patterns out of
scope for this hotfix.
Closes Task #18
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
WalkthroughThe batch settlement signing mechanism receives two updates: the EIP-712 domain's ChangesBatch Settlement EIP-712 Signing
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 updates the verifyingContract address to its checksummed version and modifies the generateNonce method to return a 0x-prefixed hex string with a more specific template literal type. A review comment suggests optimizing the hex conversion in generateNonce by using Buffer or an existing utility function to improve performance and reduce code duplication.
| const hex = Array.from(buf) | ||
| .map(b => b.toString(16).padStart(2, '0')) | ||
| .join(''); | ||
| return `0x${hex}`; |
There was a problem hiding this comment.
The current method of converting the Uint8Array to a hex string is inefficient as it involves multiple array allocations and iterations. Since this is a Node.js environment, using Buffer is significantly more performant and concise. Alternatively, you could consider importing and using the existing generateEIP3009Nonce utility from src/crypto.ts to avoid code duplication.
const hex = Buffer.from(buf).toString('hex');
return `0x${hex}`;![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/micropayment-engine.ts (1)
228-233:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse canonical network domain config instead of a hardcoded
verifyingContract.Line 232 still hardcodes the domain address and force-casts it with
as Address. That can drift from canonical network config and sign against the wrong EIP-712 domain after address changes. PullverifyingContractfrom the shared network/domain constants instead.Proposed fix
+import { EIP712_DOMAINS } from './networks.js'; ... const domain = { name: 'PayBot', version: '1', chainId: 8453, - verifyingContract: '0x50b0f7224fFC5F4F7685DbcE1B8b7e7B8b8A4A23' as Address, + verifyingContract: EIP712_DOMAINS['eip155:8453'].verifyingContract, };As per coding guidelines, "No hardcoded private keys, secrets, or wallet addresses."
🤖 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/micropayment-engine.ts` around lines 228 - 233, The domain object currently hardcodes verifyingContract and force-casts it, which can drift from the canonical network config; replace the literal '0x50b0f...' value in the domain declaration with the shared/canonical domain/network constant (e.g., the exported verifyingContract or domain value from your network/domain constants), remove the unsafe 'as Address' cast, and ensure the domain uses that imported constant (keeping name: domain) so EIP‑712 signing always uses the canonical verifyingContract.
🤖 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.
Outside diff comments:
In `@src/micropayment-engine.ts`:
- Around line 228-233: The domain object currently hardcodes verifyingContract
and force-casts it, which can drift from the canonical network config; replace
the literal '0x50b0f...' value in the domain declaration with the
shared/canonical domain/network constant (e.g., the exported verifyingContract
or domain value from your network/domain constants), remove the unsafe 'as
Address' cast, and ensure the domain uses that imported constant (keeping name:
domain) so EIP‑712 signing always uses the canonical verifyingContract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eae67023-7c1e-4529-bad1-59373d17b3b6
📒 Files selected for processing (1)
src/micropayment-engine.ts
|
Merge authorization per .claude/rules/automated-pr-merge-authority.md (7th application):
Closes Task #18. Unblocks Story 15 Track B (5 previously-failing tests should pass once @dev resumes on rebased branch). Pattern note: 3rd product bug discovered today by AIOX agents driving forward on coverage/test work:
Non-blocking observations from @qa (backlog candidates):
|
…ory 15) (#38) Closes Task #15. Builds on Story 15.1 (PR #37 @ 23694dd) which unblocked this work by fixing the EIP-55 checksum + 0x-prefix issues in src/micropayment-engine.ts. Track A — extend tests/x402-v2.test.ts (tests #14-#26) - on402Response: happy / non-402 status / missing+malformed header - submitPayment: 200 OK / non-2xx / network error rewrap - verifyReceipt: 200+verified / non-2xx / network error (no throw) - createPaymentIntentHeader: happy / optional fields undef - negotiatePaymentIntent: happy / TODO branch locked - src/x402-v2.ts: 63.74% → 97.32% line coverage Track B — create tests/micropayment-engine.test.ts (B-1 through B-18) - constructor (3): 0x-guard, defaults, custom thresholds - queuePayment (3): paymentId shape, usdToBaseUnits errors, auto-settle - batchPayments (3): EIP-712 BatchSettlement sign / missing IDs / skipGas - getGasEstimate (2): 6-decimal USD, inverse scaling - setBatchWindow (2): s→ms conversion, window-boundary observable - getQueueStatistics (2): empty zeroes, multi-window aggregation - clearOldPayments (2): nothing-to-clear, settled+old removal - getPaymentStatus (1): find-or-undefined cross-window - src/micropayment-engine.ts: 0% → 100% line coverage Track C — .github/workflows/ci.yml - Add `Coverage gate` step (npm run coverage) between Run tests + Build - Add `Upload coverage report` step (actions/upload-artifact@ea165f8 v4) with if: always(), 14-day retention, per matrix Node version Test count: 115 baseline + 13 Track A + 18 Track B = 146 tests, all PASS Coverage: global 97.52% lines / 86.75% branches / 100% funcs / 97.52% stmts (exceeds vitest.config.ts thresholds 80/80/70/80 → npm run coverage exit 0) Determinism: all tests use vi.useFakeTimers + vi.setSystemTime + frozen Math.random; fetch is stubbed via vi.stubGlobal; zero real network I/O. Verbatim source preservation: 0 changes to src/ (verified git diff --stat). Zero skipped tests, zero .todo/xit/xdescribe/.only. Story chain: full SINKRA — @sm draft (v0.1) → @po GO-conditional → @sm patch (v0.2) → @dev attempt 1 → bug escalation → @aios-master roundtable → Story 15.1 source fix (PR #37) → @dev resume (this PR) → @qa pending → @devops pending merge.
1 participant
Summary
MicropaymentEngine.signBatchSettlement()was failing on every call under viem 2.49+ because of two related bugs insrc/micropayment-engine.ts:verifyingContractliteral at line 232 had a malformed EIP-55 checksum (4 letter case-swaps). viem 2.49+ rejects non-canonical checksums withInvalidAddressError.generateNonce()returned an un-prefixed 64-char hex string, then the call site cast it as`0x${string}`— a type-system lie. Even with the checksum fixed, viem threwBytesSizeMismatchError: expected bytes32, got bytes64athashTypedData.Both fixes are required to unblock Story 15 Track B (5 failing tests: B-3, B-6, B-7, B-9, B-15).
Root cause analysis
Why no one caught it before Story 15: the only call path that exercises this code is
batchPayments()→signBatchSettlement(), and the file had 0% test coverage prior to Story 15. The malformed checksum and lying nonce cast were silently broken for the entire lifetime of the module.The 4 character case-swaps (primary):
Canonical checksum was derived programmatically, not hand-typed:
The lying type cast (secondary):
The redundant
as \0x${string}`` cast at line 235 is harmless after this change and was left untouched to keep the diff minimal.Audit pass (Task 15.1.3)
Grepped
src/micropayment-engine.tsforas \0x${string}`casts and hardcoded addresses (0x[A-Fa-f0-9]{40}`):config.walletPrivateKey as \0x${string}``startsWith('0x')check at line 52 immediately before the castverifyingContractgetAddress()this.generateNonce() as \0x${string}``generateNonce()now genuinely returns0x-prefixed and the return type is honest. Cast kept (now redundant but harmless) to minimize diffp.recipient as \0x${string}``Addressfield. No new runtime validation here, but recipients enter viaqueuePayment(recipient: Address, ...)which is the type-system gate. Per AC 3 (Out-of-Scope), not fixed in this PROnly one hardcoded address in the file. Recommend a follow-up story to add runtime
getAddress()validation atqueuePaymententry, but that is out of scope here.Secondary suspect outcome
Status: FIXED (not benign).
Local verification with the primary fix applied but
generateNonceunchanged produced:After fixing
generateNonceto return0x-prefixed hex with an honest return type:Verification method
viem.getAddress()on the lowercase form → confirms the new literal byte-for-byte.MicropaymentEnginewith a deterministic key, queued a payment, calledbatchPayments(). The signing path now produces a valid 65-byte signature and 32-byte nonce. Script deleted before commit.npm run type-check— PASS (no output).npm run lint— PASS (no output).tests/micropayment-engine.test.ts(B-3, B-6, B-7, B-9, B-15) will exercise this path properly once that branch is rebased on this fix. Per Story 15.1 scope, test file is NOT included in this PR — owned by Story 15.Files changed
src/micropayment-engine.ts— 4 insertions, 3 deletions. Two fixes, one file.SINKRA chain
docs/stories/15.1.checksum-fix.mdon this paybot-sdkgetAddress().Out of scope (deferred)
getAddress()validation atqueuePaymententry (line 240 cast).as \0x${string}`` cast at line 235.Closes Task #18
🤖 Generated with Claude Code
Summary by CodeRabbit