fix: drop viem from x402 address validation#246
Conversation
jeffsmale90
left a comment
jeffsmale90
left a comment
There was a problem hiding this comment.
Hi @peterxing thanks for the contribution!
This looks good - I think we can drop @noble/hashes, and replace it with @metamask/utils (which uses @noble/hashes internally) .
The ticket does suggest implementing bespoke validation, but because we are doing checksumming, it probably makes sense to pull in the @metamask/utils dependency instead of implementing it in this package directly.
| } | ||
|
|
||
| return checksummed as Address; | ||
| } |
There was a problem hiding this comment.
I think it would make sense to add a dependency to @metamask/utils instead of @noble/hashes directly, and use the getChecksumAddress function instead of implementing it ourselves here in checksumAddress.
We could also drop the regex validation as this is implemented in getChecksumAddress in @metamask/utils - it throws Invalid hex address..
The checksum validation in getAddress looks good though.
| if ( | ||
| address !== lowerAddress && | ||
| address !== upperAddress && | ||
| `0x${address}` !== checksummedAddress |
There was a problem hiding this comment.
Instead of using string interpolation here, we can just compare value !== checksummedAddress
| `0x${address}` !== checksummedAddress | |
| value !== checksummedAddress |
|
Addressed the review note in follow-up commit Changes:
Verification run locally:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit b06ec40. Configure here.
|
Addressed the Cursor Bugbot follow-up in commit The issue was that the all-uppercase no-checksum branch compared Verification run locally:
No wallet activity, signing, payments, or live x402 calls were used. |
jeffsmale90
left a comment
jeffsmale90
left a comment
There was a problem hiding this comment.
Looks good - a couple things that can be removed, and replaced with references to @metamask/utils
| const HEX_REGEX = /^0x[0-9a-fA-F]*$/u; | ||
|
|
||
| /** | ||
| * Check whether a value is a 0x-prefixed hexadecimal string. | ||
| * | ||
| * @param value - Value to inspect. | ||
| * @returns True when the value is hex data. | ||
| */ | ||
| export function isHex(value: unknown): value is Hex { | ||
| return typeof value === 'string' && HEX_REGEX.test(value); | ||
| } |
There was a problem hiding this comment.
We can remove the isHex util and HEX_REGEX from here, and use the isStrictHexString function exported from @metamask/utils in all cases where we presently call isHex
It ensures non-zero length '0x' prefixed hex string.
| export type Address = `0x${string}`; | ||
| export type Hex = `0x${string}`; |
There was a problem hiding this comment.
We can also remove the Hex and Address types and use the Hex type exported from @metamask/utils in all places where we reference these types (Hex and Address are functionally identical, so I don't think there's value in distinguishing unless there's an actual difference in the type).
mj-kiwi
left a comment
mj-kiwi
left a comment
There was a problem hiding this comment.
LGTM, seems the isHex function isn't necessary
There was a problem hiding this comment.
As @jeffsmale90 said, isHex isn't needed. We can use isStrictHexString from @metamask/utils and revise the code accordingly.
| if (!isStrictHexString(payload.permissionContext)) { |
|
@peterxing You'll also need to add a changelog line to /packages/smart-accounts-kit/CHANGELOG.md |
|
Addressed the requested changelog update in commit Changes:
Verification:
I could not run |
|
Addressed the latest review notes in
Verification on this branch:
|
|
|
||
| ### Changed | ||
|
|
||
| - Use `@metamask/utils` for x402 address checksum validation instead of `viem`. ([#246](https://github.com/MetaMask/smart-accounts-kit/pull/246)) |
There was a problem hiding this comment.
| - Use `@metamask/utils` for x402 address checksum validation instead of `viem`. ([#246](https://github.com/MetaMask/smart-accounts-kit/pull/246)) |
|
Addressed the latest changelog suggestion in commit Change:
Verification:
No code paths changed. |
|
The remaining The job runs Current state after the last requested change:
I cannot fix that checkout behavior from the fork branch. If the x402 changelog entry is acceptable as-is, this likely needs a maintainer-side rerun/workflow adjustment or a label/action that skips the branch checkout path for fork PRs. |
|
Thanks for merging this. If useful for release confidence after the x402 package update, I can do a fixed-scope A$390 package-consumer readback: install from the published/built package in a clean app, verify peer dependency warnings are gone, exercise the server/client import surfaces, and report any install/release-note gaps as a concise checklist. Contact: info@transhumanism.com.au |
|
Quick release-side follow-up now that the merged x402 package change appears to be on npm ( Scope would stay narrow: clean app install, peer/dependency warning check, server/client import smoke, and a concise release-readiness checklist. No wallet activity, signing, payments, live x402 calls, or private credentials involved. Contact: info@transhumanism.com.au |
|
MetaMask's June 8 Agent Wallet early-access launch creates a timely release-side check for this x402 dependency change: https://metamask.io/news/introducing-metamask-agent-wallet If useful, I can do a fixed-scope A$390 agent-wallet/x402 consumer readback against the published package and current docs surface: clean install/import smoke, peer/dependency warning check, x402 server/client example sanity review, and a short checklist of any packaging or release-note gaps relevant to agent-wallet builders. Scope stays read-only and local/simulated: no wallet activity, signing, payments, live x402 calls, funds, private credentials, or early-access account access. Contact: info@transhumanism.com.au |
3 participants
Summary
viemaddress/hex helpers in@metamask/x402with local Ethereum validation helpers.viemfrom@metamask/x402peer/dev dependencies and add focused checksum regression coverage.Verification
corepack yarn workspace @metamask/x402 lintcorepack yarn workspace @metamask/x402 typecheckcorepack yarn workspace @metamask/x402 test— 3 files / 21 tests passedcorepack yarn workspace @metamask/x402 buildgit diff --checkCloses #243
Note
Low Risk
Dependency swap and localized validation helpers in the x402 package; behavior is covered by existing and new tests with no auth or payment flow changes.
Overview
@metamask/x402 no longer depends on viem for address and hex handling.
@metamask/utilsis added as a direct dependency, and a localgetAddresshelper inethereum.tsvalidates EIP-55 checksums (rejecting invalid mixed-case) while still accepting all-lowercase or all-uppercase inputs for normalization.Client and server paths now import
getAddressandHexfrom that helper /@metamask/utilsinstead of viem. DelegationpermissionContextchecks useisStrictHexString(replacing viem’sisHex), which still rejects empty0x. Facilitator address typing on the server usesHex[]instead of viem’sAddress.viem is removed from peer and dev dependencies; the changelog and lockfile reflect the swap. Tests cover all-uppercase facilitator normalization and invalid mixed-case checksum errors.
Reviewed by Cursor Bugbot for commit 3d23e9d. Bugbot is set up for automated code reviews on this repo. Configure here.