Conversation
Code Review ReportProject: x402 Payment Protocol PR OverviewBranch Information
Commit History
Review SummaryVerdict
Findings at a Glance
SummaryThis PR is a documentation-only change — it adds seven new Markdown files covering the x402 payment protocol: a knowledge base of hard-won debugging solutions ( The documentation is generally well-structured and technically detailed, covering critical edge cases such as TRON address format conversions, GasFree deadline bounds, and EIP-712 primary type handling. However, several issues merit attention before merge: internal BankOfAI infrastructure URLs are hardcoded into public-facing spec documentation (operational security risk), the Change SummaryGroup 1 — Knowledge Base
Purpose: Captures accumulated debugging knowledge to prevent repeat errors. Group 2 — Protocol & Role Specifications
Purpose: Defines the canonical protocol contract, HTTP headers, all wire-format structures, and the expected behavior of each participant. Group 3 — Payment Scheme Specifications
Purpose: Scheme-by-scheme signing specs, business rules, client/server/facilitator behavior, and API contracts. Group 4 — Configuration & Registry
Purpose: Single authoritative reference for all network-specific addresses and token metadata. Detailed FindingsMajor[MJ-01] Internal Infrastructure URLs Hardcoded in Public Spec Documentation
Description
Code ## GasFree API Base URLs
| Network | URL |
|---------|-----|
| `tron:mainnet` | `https://facilitator.bankofai.io/mainnet` |
| `tron:shasta` | `https://facilitator.bankofai.io/shasta` |
| `tron:nile` | `https://facilitator.bankofai.io/nile` |Recommendation Separate operational configuration from the protocol spec. The spec should describe
the *format* of the GasFree API Base URL (e.g., "a base URL provided at client
initialization time") rather than hardcoding deployment-specific values.
Move actual URLs into a separate configuration guide or environment-specific config
files, with a note like:
> The GasFree API base URL is an operator-specific configuration value.
> Reference implementations use `https://facilitator.bankofai.io/{network}`.
> Configure via the `GASFREE_API_BASE_URL` environment variable or equivalent.[MJ-02]
|
| Property | Value |
|---|---|
| Severity | Major |
| Category | Security |
| File | docs/specs/exact.md : Lines 797–799 |
Description
The
exactscheme spec states: "Theexactscheme delegates all verification to the facilitator. Server-side signature verification returnstrue(passthrough) — the on-chain call itself serves as verification."This is presented as a matter-of-fact design choice without any risk discussion. The implication is significant: a server implementing
exactwill accept a payment payload and respond with 200 without performing any local cryptographic verification. The entire security model depends on the facilitator settlement call succeeding. If the facilitator is unavailable, slow, or misbehaves, the server has no local defense. Implementors reading this spec may not realize they are accepting an elevated trust assumption on the facilitator.
Code
### Server Verification
The `exact` scheme delegates all verification to the facilitator. Server-side
signature verification returns `true` (passthrough) — the on-chain call itself
serves as verification.Recommendation
Add an explicit security note explaining the trust model and its implications:
> **Security note**: Because server-side verification is passthrough, the server
> implicitly trusts the facilitator to reject invalid or already-used signatures.
> Servers using this scheme MUST ensure the facilitator connection is trusted and
> authenticated. This differs from `exact_permit` where the server performs local
> signature recovery before delegating settlement.
>
> Replay protection relies entirely on the token contract rejecting reused nonces
> on-chain. Servers MUST treat a failed facilitator settlement response as a
> payment rejection and MUST NOT serve the resource.[MJ-03] gasfreeAddress Extensions Field Referenced in Scheme Spec But Absent from Wire-Format Protocol Spec
| Property | Value |
|---|---|
| Severity | Major |
| Category | Documentation |
| File | docs/specs/exact-gasfree.md : Line 547; docs/specs/protocol.md : Lines 929–934 |
Description
exact-gasfree.mdStep 8 of the Client Behavior section states: "Build PaymentPayload withgasfreeAddressin extensions." However,protocol.md'sPaymentPayloadstructure shows"extensions": {}with no defined fields. There is no documentation of what key thegasfreeAddressis stored under, what its type/format is (Base58? EVM hex?), or how the facilitator reads it from the extensions object. An implementor following both specs cannot build a conformant client without reading the source code.
Code
# exact-gasfree.md (line 547)
8. Build PaymentPayload with `gasfreeAddress` in extensions
# protocol.md (lines 929-934) — PaymentPayload definition
"extensions": { }Recommendation
Extend the PaymentPayload definition in `protocol.md` to document the
`exact_gasfree` extension:
```json
"extensions": {
"gasfreeAddress": "(exact_gasfree only) custodial wallet address (EVM hex format)"
}And update exact-gasfree.md Step 8 to be explicit:
8. Build PaymentPayload with `extensions.gasfreeAddress` set to the
EVM-hex-format gasFreeAddress returned by the GasFree API account info endpoint
[MJ-04] merchantSignature Field in PaymentPayload Is Undocumented
| Property | Value |
|---|---|
| Severity | Major |
| Category | Documentation |
| File | docs/specs/protocol.md : Lines 928–934 |
Description
The
PaymentPayloadwire-format definition includes an"merchantSignature": "(optional) string"field. No documentation anywhere in the PR explains what this field contains, when it is populated, which party creates it, what is signed, or how it is verified. For implementors building clients or servers to this spec, this field is a black box. Leaving undocumented optional fields in a public wire-format spec invites misuse and interoperability failures.
Code
"payload": {
"signature": "EIP-712 signature hex string",
"merchantSignature": "(optional) string", ← undocumented
"paymentPermit": PaymentPermit
}Recommendation
Either:
(a) Document the field fully: what it signs, who creates it, when it is required,
and how it is verified; OR
(b) Mark it as reserved/internal with a note:
`"merchantSignature": "(reserved, not used in current schemes)"` and add a
note that it is present for forward compatibility; OR
(c) Remove it from the spec entirely if it is not yet implemented.Minor
[MN-01] exact-gasfree.md API Contract Does Not Document data: null Behavior
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation |
| File | docs/specs/exact-gasfree.md : Lines 365–370; docs/solutions.md : Lines 69–84 |
Description
solutions.mdentry #4 documents a critical GasFree API behavior: responses can return{"code": 200, "data": null}during early polling. This behavior is absent fromexact-gasfree.md's API Contract section, which only shows a successful response envelope with"data": { ... }. An implementor reading only the scheme spec (the authoritative reference) would not handle this case and would experience the exact crash documented in solutions.md.
Recommendation
Add a note to the common response envelope description in
exact-gasfree.md:**Note**: `data` may be `null` even when `code` is `200`. This occurs when the API is polled shortly after submission before the transaction record is available. Treat `data: null` as "not yet available" and retry on the next poll interval. Do NOT treat it as an error or a failure.
[MN-02] exact_permit EIP-712 Domain Omits version Field — Inconsistency Unexplained
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation |
| File | docs/specs/exact-permit.md : Lines 588–603; docs/specs/exact-gasfree.md : Lines 312–329 |
Description
The
exact_permitEIP-712 domain only containsname,chainId, andverifyingContract— there is noversionfield. By contrast,exact_gasfreeincludesversion: "V1.0.0"andexactincludesversionfrom the token registry. The spec does not explain this divergence. Developers implementing multiple schemes may incorrectly add aversionfield toexact_permitdomain (which would change the domain separator and invalidate signatures).
Recommendation
Add an explicit note to
exact-permit.md:**Note**: The `exact_permit` EIP-712 domain deliberately omits the `version` field. Adding `version` to the domain would produce a different domain separator and invalid signatures. Do not add `version` even if your EIP-712 library includes it by default — check for a `skipVersion` or equivalent option.
[MN-03] PaymentPermit Contract Coverage Gap — No Addresses for Ethereum Mainnet or Sepolia
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation |
| File | docs/specs/config.md : Lines 154–178 |
Description
The Network Identifiers table lists four EVM networks (
eip155:1,eip155:11155111,eip155:56,eip155:97). However, the PaymentPermit Contract Addresses table only coverseip155:97(BSC Testnet) andeip155:56(BSC Mainnet). Ethereum Mainnet (eip155:1) and Sepolia (eip155:11155111) have no contract entries. The spec does not explain whether this is intentional (contracts not yet deployed) or an omission, leaving ambiguity about EVM network support scope.
Recommendation
Add a clarifying note:
**Note**: PaymentPermit contracts are currently deployed on BSC networks only. Ethereum Mainnet (`eip155:1`) and Sepolia (`eip155:11155111`) are listed as recognized network identifiers but are not yet supported for `exact_permit`. The fallback zero address applies to these networks; attempting payments on them will fail.
[MN-04] TRON vs EVM Approval Max Amount Discrepancy Not Explained
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation |
| File | docs/specs/exact-permit.md : Lines 659–661 |
Description
The spec specifies different maximum approval amounts for TRON vs EVM:
- EVM:
approve for 2^256 - 1(standard max uint256)- TRON:
approve for 2^160 - 1(max uint160)No rationale is given for the TRON difference. TRON's
approvefunction in TRC-20 typically accepts uint256 like EVM. Using2^160 - 1is a non-obvious constraint; without explanation, implementors may assume it is an error and use2^256 - 1, which may or may not work.
Recommendation
Add a brief rationale:
- TRON: approve for `2^160 - 1` (max uint160 — TRON TRC-20 token contracts cap approval values at 160-bit integers in their internal representation; using `2^256 - 1` may cause overflow in some contract implementations)
[MN-05] solutions.md References Internal Slash Command Tooling Without Context
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation |
| File | docs/solutions.md : Line 11 |
Description
The first line of the knowledge base states: "Use
/x402:compoundto add new entries after solving a problem." This references an internal slash command (presumably a Claude Code skill) that is meaningless to external contributors or anyone without that specific tooling environment. If this file is read in a standard GitHub context, the instruction is confusing and non-actionable.
Recommendation
Either remove the tooling-specific instruction from the document header, or add context:
<!-- For internal contributors using Claude Code: run /x402:compound to add new entries. For others: follow the template at the bottom of this file to add entries manually. -->
Suggestions
[S-01] Cross-Reference solutions.md Entries to Spec Files
File: docs/solutions.md
Description: Each of the 6 solution entries is directly related to a specific spec behavior (e.g., entry #1 relates to address handling in exact-gasfree.md and exact-permit.md). There are no cross-references.
Suggestion: Add a **See also** line to each entry pointing to the relevant spec section. Example: **See also**: [exact-gasfree.md — TIP-712 Signing Specification](./exact-gasfree.md#tip-712-signing-specification)
[S-02] exact.md Should Note ERC-3009 Replay Protection
File: docs/specs/exact.md
Description: ERC-3009 nonces are one-time-use. The spec documents that the nonce is "random 32-byte value" but does not explain the replay protection model — the token contract records used nonces and rejects reuse. Implementors need to understand this is the only replay guard.
Suggestion: Add a security note: "The random nonce provides replay protection: the token contract records each used (from, nonce) pair and reverts on reuse. Clients MUST generate a fresh cryptographically random nonce per payment attempt."
[S-03] Clarify CheapestTokenSelectionStrategy Normalization Logic
File: docs/specs/roles.md
Description: The spec mentions this strategy "selects the requirement with the lowest cost, normalized by token decimals" but doesn't explain the normalization formula. With multiple tokens (USDT 6 decimals vs USDT ERC-20 18 decimals), the normalization must be precise.
Suggestion: Add a brief formula: "Cost is computed as amount / 10^decimals, where decimals is from the token registry. The requirement with the smallest normalized cost is selected."
[S-04] Document PaymentPermitContext Nonce Format
File: docs/specs/protocol.md and docs/specs/exact-permit.md
Description: The PaymentPermitContext.nonce is described as "random value (as string)" in roles.md and as "string" in the protocol wire format. It's unclear whether this is a numeric string, a hex string, or an arbitrary opaque string, and what range it should cover.
Suggestion: Specify the nonce format explicitly: "A random uint256 value encoded as a decimal string (e.g., '12345678901234567890'). Clients MUST use the nonce provided by the server verbatim in their signature."
Positive Observations
| Area | Observation |
|---|---|
| Structured bug documentation | solutions.md follows a disciplined format (Symptom → Root cause → Fix → Rule → Ref) that makes it easy to scan and apply lessons |
| EIP-712 primary_type explicitness | All three scheme specs call out primary_type explicitly and document it separately, directly addressing the class of bug documented in solutions.md entry #5 |
| State machine documentation | exact-gasfree.md includes both state and txnState state machines, plus explicit terminal success/failure conditions — this level of specificity prevents ambiguous polling implementations |
| Deadline clamping spec | The mainnet vs. testnet deadline bounds are documented precisely (including 5s padding), with both a table and the business rule description |
| Fee calculation formula | The maxFee formula in exact-gasfree.md covers all cases (transferFee, facilitatorFee, activateFee, zero-fee fallback) unambiguously |
| Address format reference | config.md includes a dedicated Address Formats table covering all three formats (TRON Base58, TRON raw hex, EVM hex) with conversion rules |
| Anti-tampering documentation | roles.md explicitly calls out the server's anti-tampering check as a separate step, preventing server-side requirement substitution attacks |
| Token registry defaults | Token version defaulting to "1" is documented as a global default in config.md, preventing silent domain separator mismatches |
Checklist Results
| Category | Items Checked | Pass | Fail | N/A | Notes |
|---|---|---|---|---|---|
| Correctness | 8 | 6 | 2 | 0 | Gaps: gasfreeAddress extension undocumented; data:null behavior missing from scheme spec |
| Security | 10 | 7 | 2 | 1 | Gaps: hardcoded internal URLs; passthrough verification not flagged as security trade-off |
| Performance | 7 | 0 | 0 | 7 | N/A — documentation only |
| Code Quality | 10 | 7 | 2 | 1 | Gaps: undocumented merchantSignature; tooling reference in solutions.md |
| Testing | 7 | 0 | 0 | 7 | N/A — documentation only |
| Documentation | 6 | 3 | 3 | 0 | Gaps: exact_permit domain version omission unexplained; address coverage gaps; nonce format ambiguous |
| Compatibility | 5 | 4 | 1 | 0 | PaymentPermit contract addresses missing for Ethereum Mainnet and Sepolia |
| Observability | 4 | 0 | 0 | 4 | N/A — documentation only |
Disclaimer
This is an automated code review. It supplements but does not replace human review. The reviewer analyzed only the diff between the specified branches. Runtime behavior, integration testing, and deployment impact are not covered.
Report generated by Code Review Skill v1.0.0
Date: 2026-04-13
2 participants
Description
Tests
Checklist