Repository navigation
docs(x402): use EIP-55 checksummed addresses and require them in edited content - #2356
Conversation
GigaHierz
left a comment
There was a problem hiding this comment.
head 54cb1c2 · EIP-55 recomputed (cast + viem getAddress) matches all 3 changed lines; eth_getCode returns contract code, symbol USDC, decimals 6 on chain 42220; repo scan 1595 addresses, 0 bad checksums; broken-links --check-redirects pass; check-orphans clean; CI green
Verdict: REQUEST-CHANGES
Setup gate: API head equals the fetched ref (no STALE). Branch is 2 ahead / 7 behind main; main changed x402.mdx in different hunks (a Note and the "Trust and other facilitators" section) and git merge-tree against current main is clean. Reviewed the diff against current main.
Findings
- LOW · build-on-celo/build-with-ai/x402.mdx:196 and AGENTS.md:97 · The PR adds the rule "Every address is written in its EIP-55 checksummed form", but the USDT address in the same table the PR edits (
0x48065fbbe25f71c9282ddf5e1cd6d6a887483d5e) is all-lowercase, so the page violates the rule the PR introduces. Failure scenario: the next reviewer applying the new rule to this page flags line 196; a reader sees USDC checksummed and USDT not and cannot tell whether the lowercase one is a typo. (Not a functional break: viemisAddressand ethers both accept all-lowercase; verified.) Fix shape: change line 196 to0x48065fbBE25f71C9282ddf5e1cD6D6A887483D5e(cast output; on-chain symbolUSD₮, decimals 6), or narrow the AGENTS.md sentence to mixed-case addresses and new addresses. - LOW · AGENTS.md:97 · The rule as worded ("Every address") is contradicted by 495 existing all-lowercase address occurrences in non-submodule content (for example snippets/tokens.mdx, home/bridged-tokens/*, home/protocol/challengers.mdx). Failure scenario: the rule is unenforceable as written and either gets ignored or invites drive-by churn. Fix shape: scope the sentence to "new and edited addresses" (as the heading-case rule is scoped), or file and link a follow-up issue for the lowercase sweep.
- LOW · PR body · It lists the three changed places as "the server example, the token table and the client example". Line 210 is a
mainnetAcceptsroute-config snippet under "Celo Configuration", not a client example; the page has no client example containing this address. Fix shape: edit the body to say "server example, token table and the mainnet config snippet".
Verified good
- EIP-55 of 0xceba9300f2b948710d2653dd7b07f33a8b32118c computed independently with
cast to-check-sum-address:0xcebA9300f2b948710d2653dD7B07f33A8B32118C. The 3 changed occurrences (x402.mdx:133, :195, :210) match it character for character. - Repo-wide case-insensitive grep: 15 occurrences of this address outside submodules (fee-currencies, stablecoin-contracts, using-fee-abstraction x2, build-with-ai/overview, minipay overview x2, build-with-farcaster, mpp, x402 x3, plus explorer URLs), all with the identical checksummed casing. No stragglers, none in submodules.
- Whole-repo scan with viem
getAddress(md/mdx/json/js/jsx, submodules excluded): 1595 address occurrences, 0 mixed-case checksum failures, 0 all-uppercase. Consistent with the body's "none fail" claim. The body's 1,100 figure was not reproduced exactly (counting scope differs) but the conclusion holds. - On-chain (https://forno.celo.org, chainId 42220): the address has contract code,
symbol()= "USDC",decimals()= 6. The Celo Sepolia USDC at line 222 is correctly checksummed, symbol USDC, decimals 6 on the Sepolia RPC. - The body's failure claims are true: viem
isAddress(old)is false, ethersgetAddress(old)throws "bad address checksum"; both succeed on the new casing. - Agrees with the AGENTS.md section 7 pages that carry the address (stablecoin-contracts, fee-currencies, mpp, minipay overview).
- Section 5 comments intact: server example has "Celo mainnet" and "USDC has 6 decimals" comments beside the asset; line 210 reads "Celo mainnet USDC" with a decimals comment; the table has a Decimals column under the Celo section.
mint broken-links --check-redirects: success.scripts/check-orphans.sh: no orphan pages.gh pr checks: broken-links pass, GitGuardian pass, StepSecurity pass, Mintlify Deployment skipping (not a code failure).- Text-only change inside code blocks and a table; no layout change, so no
mint devpass was needed.
What the PR got right
A correct, minimal fix on a money-adjacent page: the wrong-checksum casing was a real copy-paste break and is eliminated repo-wide, verified with two independent tools. Adding the rule to AGENTS.md with the exact error strings and the cast command fits the repo's troubleshooting-first style. One concern, Conventional Commit title.
Merge-order hazards
Main has since edited x402.mdx (the Note near line 74 and the "Trust and other facilitators" section near line 242); hunks do not overlap this PR and a trial merge is clean. AGENTS.md is untouched on main since the merge base. Any other open PR that adds the USDC address with the old casing would reintroduce the defect; none found on main.
54cb1c2 to
046b428
Compare
|
All three are in 046b428, rebased onto current main.
|
GigaHierz
left a comment
There was a problem hiding this comment.
head 046b428 · fetch matched headRefOid (API == refs/remotes/pr/2356); branch 4 commits behind main (main already has the learn/ move, #2367; no build/ move yet), merge-tree clean (AGENTS.md and x402.mdx auto-merge, hunks disjoint); on a merge of head into main a12466b: mint broken-links --check-redirects pass, check-orphans.sh pass, CI green (broken-links, GitGuardian, StepSecurity); mint dev on :3412: 0 console errors on /build-on-celo/build-with-ai/x402 at 1280 and 412x915, no horizontal scroll
Verdict: REQUEST-CHANGES
Scope: AGENTS.md (+1 line) and build-on-celo/build-with-ai/x402.mdx (4 lines changed: USDC at :133, :195, :210 and USDT at :196), three commits on 92b8cc6.
Status of earlier findings
- The new rule is broken by the page it edits (USDT lowercase at x402.mdx:196): FIXED. x402.mdx:196 is now
0x48065fbBE25f71C9282ddf5e1cD6D6A887483D5e.cast to-check-sum-address 0x48065fbbe25f71c9282ddf5e1cd6d6a887483d5ereturns exactly that string; on https://forno.celo.org (chain 42220) the contract returnssymbol()= "USD₮" anddecimals()= 6. - The rule as worded ("Every address") contradicts about 495 lowercase occurrences: FIXED. AGENTS.md:97 now reads "Write new and edited addresses in their EIP-55 checksummed form; ... Do not sweep old pages just to recase addresses; fix the ones on pages you are already editing", the same scope as the heading-case rule. The remaining lowercase USDT occurrences (using-fee-abstraction.mdx:62, fee-currencies.mdx, stablecoin-contracts.mdx, build-on-minipay/overview.mdx) are on pages this PR does not edit, which the rule allows.
- Body called line 210 a "client example": FIXED. The body now says "
mainnetAcceptsroute config under 'Celo Configuration'", which matches x402.mdx:206-211.
New findings
- LOW · PR title · The title still reads "docs(x402): use the EIP-55 checksummed USDC address", but the squash commit on main will also checksum the USDT address and add a repo-wide rule to AGENTS.md. A reader scanning
git logfor the AGENTS.md rule, or for the USDT change, will not find it, and the title is the only text that lands on main.- Fix shape: retitle, for example
docs(x402): use EIP-55 checksummed addresses and require them in edited content. No commit needed; a title edit resolves it.
- Fix shape: retitle, for example
Verified good
- EIP-55 independently recomputed with
cast to-check-sum-address:0xcEBA9300f2b948710d2653dD7B07f33A8B32118C(old casing) maps to0xcebA9300f2b948710d2653dD7B07f33A8B32118C, so the old casing was a genuine wrong-checksum string. All three USDC occurrences changed here match the new casing character for character. - Repo-wide, case-insensitive: 15 occurrences of the USDC address outside submodules, all
0xcebA9300f2b948710d2653dD7B07f33A8B32118C; none with the old casing (grep -E '0xcEBA9300'finds nothing on the PR head). - Body claim "every mixed-case address in the repo passes": I took every unique 0x+40-hex string in md/mdx/json/js/jsx outside submodules (444), filtered to the 263 that are mixed case, and ran each through
cast to-check-sum-address; all 263 equal their input, none fail. I did not re-run the viem and ethers error-string claims ("ethers throws bad address checksum", "viem isAddress returns false"); they rest on the earlier review's run plus the checksum mismatch above. - On-chain (https://forno.celo.org, chain 42220): USDC at the new casing returns
symbol()= "USDC",decimals()= 6. Table rows keep the Decimals column; code comments keep "Celo mainnet USDC" / "USDC has 6 decimals" per AGENTS.md section 5. - Rendered page at 1280 and 412x915 (isolated pages, URL port checked): the three address strings appear in the new casing, USDT row shows the checksummed address, 0 console errors, scrollWidth equals viewport width.
- AGENTS.md line: states how to get the value (
cast to-check-sum-address <address>), names both error strings, and is scoped to new and edited content. No promotional wording, no roadmap language.
What the PR got right
- A real, minimal copy-paste fix on a payments page, with the rule added next to the code-example rules where agents and authors will see it, and with scope narrowed so it does not invite churn.
- Fixed the USDT inconsistency in the same table instead of leaving the page violating its own rule.
- Each earlier finding was answered with a commit and a precise comment.
Remaining work is a title edit.
Merge-order hazards
- #2368 (head 961edef, build/ move):
git merge-treeof this PR with that head is clean (x402.mdx rename followed). No action. - #2342 also edits build-on-celo/build-with-ai/x402.mdx: merge-tree with this PR is clean. Its branch still carries the old USDC casing at :133/:195/:210 and lowercase USDT at :196, but it does not touch those lines, so whichever merges second keeps the checksummed values. If #2342 adds any new address lines before it merges, check their casing.
- #2364 and #2369 touch AGENTS.md: merge-tree of each with this PR is clean. The added line sits in section 5 and does not overlap their hunks.
|
Retitled, so the squash commit names both the USDT change and the AGENTS.md rule. |
GigaHierz
left a comment
There was a problem hiding this comment.
head 046b428 · fetch matched headRefOid; main 7b8f97c (#2368 layout); merge-tree clean; trial merge: cast EIP-55 on all 3 page addresses OK, repo sweep 372 unique addresses / 263 mixed-case / 0 bad checksum, old casing cEBA9300 0 hits, broken-links --check-redirects pass, check-orphans pass, CI green, mint dev (smoke test) 0 console errors
Verdict: APPROVE
Status of earlier findings
- Title said USDC only: FIXED. The title now reads
docs(x402): use EIP-55 checksummed addresses and require them in edited content. The squash commit carries three changes (USDC casing at x402 :133/:195/:210, USDT casing at :196, one AGENTS.md rule at :97) and the title names both the addresses and the rule. The body matches the diff: it describes the USDC and USDT fixes, themainnetAcceptsroute config, and the rule scoped to new and edited content. - Earlier lowercase-USDT finding, "Every address" wording finding, and "client example" body wording: still fixed (verified again on the trial merge, see below).
- No other open review threads, inline comments, or failing checks (broken-links, GitGuardian, StepSecurity pass; Mintlify Deployment skipping).
New findings
None.
Verified good
- Trial merge of head into current main (
git merge-tree --write-tree origin/main refs/remotes/pr/2356, exit 0): the PR'sbuild-on-celo/build-with-ai/x402.mdxedit follows the rename tobuild/agents/x402.mdxautomatically. Resulting diff against main is 2 files, +5/-4:AGENTS.mdandbuild/agents/x402.mdx. Nobuild-on-celo/path remains in the merged tree. cast to-check-sum-addresson every address onbuild/agents/x402.mdx(0xcebA9300f2b948710d2653dD7B07f33A8B32118C, 0x48065fbBE25f71C9282ddf5e1cD6D6A887483D5e, 0x01C5C0122039549AD1493B8220cABEdD739BC44E): each equals its input.- Repo-wide sweep on the trial merge (md/mdx/json/js/jsx, submodules and node_modules excluded): 372 unique 0x+40-hex strings, 263 mixed-case, every one equals its
castchecksum. No remaining0xcEBA9300...casing anywhere. This confirms the body's "none fail" claim. - AGENTS.md:97 says how to get the value, names both error strings, and is scoped to new and edited addresses ("Do not sweep old pages"). It does not contradict the ~lowercase addresses still on untouched pages.
- Rendered
/build/agents/x402withmint devon :3531 (smoke test): three addresses show in checksummed form, 0 console errors, no horizontal scroll at 1280. The only console entry is a warning. mint broken-links --check-redirects: success.bash scripts/check-orphans.sh: no orphan pages.- Code-example rules intact: network and decimals comments beside each address, Decimals column in the table.
What the PR got right
A minimal fix for a real copy-paste break on a payments page, the rule placed where authors read code-example guidance, scope kept narrow so it does not invite churn, and every earlier finding answered with a commit or an edit.
Merge-order hazards and rebase work
- Layout move (#2368): no rebase needed for content.
git merge-tree origin/main refs/remotes/pr/2356is clean because git follows thebuild-on-celo/build-with-ai/x402.mdxtobuild/agents/x402.mdxrename. GitHub's merge button will do the same. An optional rebase onto main would make the branch's file paths match, but is not required. - Any other open PR that adds an address to the x402 page or AGENTS.md should be checked for casing by whichever merges second (not re-tested here beyond this PR's own merge-tree).
046b428 to
7f4a25a
Compare
GigaHierz
left a comment
There was a problem hiding this comment.
Re-approving after a rebase onto the new build/ layout (7f4a25a). The content is the head I approved (046b428): the same two files change, and the only difference is the page path (build-on-celo/build-with-ai/x402.mdx is now build/agents/x402.mdx). mint broken-links --check-redirects and the orphan check pass on the rebased branch.
The x402 page wrote Celo mainnet USDC as
0xcEBA9300f2b948710d2653dD7B07f33A8B32118Cin the server example, the token table and themainnetAcceptsroute config under "Celo Configuration". That casing fails the EIP-55 checksum, so code copied from the page breaks: ethers throwsbad address checksumand viem'sisAddressreturnsfalse. This switches to0xcebA9300f2b948710d2653dD7B07f33A8B32118C, the casing every other page already uses. It also checksums the USDT address in the same table and adds an AGENTS.md rule asking for checksummed addresses in new and edited content, scoped like the heading-case rule.cast to-check-sum-address 0xcEBA9300f2b948710d2653dD7B07f33A8B32118Creturns0xcebA9300f2b948710d2653dD7B07f33A8B32118C. For USDT,cast to-check-sum-address 0x48065fbbe25f71c9282ddf5e1cd6d6a887483d5ereturns0x48065fbBE25f71C9282ddf5e1cD6D6A887483D5e, and on Celo mainnet that contract returnssymbol()USD₮anddecimals()6. I also ran every mixed-case address in the repo through viem'sgetAddress(submodules excluded), and on this branch none fail.The reviewer counted about 495 all-lowercase addresses elsewhere in the repo. Lowercase passes both ethers and viem, so those don't break copied code. The new rule leaves them alone until someone edits the page they're on.
mint broken-links: success no broken links found.bash scripts/check-orphans.sh: No orphan pages found.