[#1240] Add SIWE verification lib (full EIP-4361 validation)#1275
Conversation
Pure verification lib using siwe@2 package: validates signature, domain, URI, chain ID, statement, and issuedAt freshness against config values. Returns typed ok/error result. 9 unit tests cover all rejection paths (bad sig, wrong domain/URI/chain/statement, expired, future-dated, unparseable). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
realproject7
left a comment
There was a problem hiding this comment.
@re2 review — APPROVE ✅
Checked against issue #1240 acceptance criteria:
| Criterion | Status |
|---|---|
Valid sig within 10 min → { ok: true, address } |
✅ |
Sig older than 10 min → { ok: false, error: 'expired' } |
✅ |
| Wrong domain/URI/chainId/statement → typed error | ✅ |
Bad signature → { ok: false, error: 'invalid_signature' } |
✅ |
| Unit tests cover all rejection paths (9 tests) | ✅ |
siwe@^2.3.2 in package.json + lockfile |
✅ |
| No DB writes (pure verification) | ✅ |
| Version bump 1.30.1 → 1.30.2 (patch) | ✅ |
Code review notes:
- Validation order is correct: parse → field checks (domain, URI, chainId, statement) → freshness → crypto verify. Field checks before crypto is more efficient and safe since config values are public.
- Future-date tolerance of 1 minute (
ageMin < -1) is reasonable for clock skew. - Error mapping in the verify catch block returns
invalid_signature— safe, doesn't leak internals. - Address returned as lowercase — consistent with rest of codebase.
- Tests use Hardhat default key #0 with viem's
privateKeyToAccountfor real SIWE signing — good, exercises the actualsiwepackage.
No issues found.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The SIWE verifier shape is mostly aligned with #1240, but the dependency/lockfile update is not installable in CI. Both required checks currently fail at npm ci, so this cannot merge.
Findings
-
[high]
package-lock.jsonis out of sync withpackage.jsonafter addingsiwe; CI fails before typecheck/e2e withnpm cireporting missingutf-8-validate@5.0.10entries. The lockfile root version is also still1.30.1whilepackage.jsonis1.30.2, which confirms it was not regenerated cleanly for this change.- File:
package-lock.json:3 - Suggestion: Regenerate the lockfile from this branch with the repo npm version, commit the resulting
package-lock.json, and rerun CI.
- File:
-
[medium] #1240 explicitly requires validating that
expirationTimehas not passed, but the new tests do not cover that EIP-4361 rejection path. The code may rely onSiweMessage.verify()for it, but without a test this acceptance item can regress silently.- File:
lib/airdrop/siwe-verify.test.ts:83 - Suggestion: Add a test with a signed SIWE message whose
expirationTimeis in the past and assert the verifier rejects it, preferably normalized to the sameexpirederror used for staleissuedAt.
- File:
Decision
Request changes. Fix the lockfile/CI failure and add the missing expiration-time coverage, then request re-review.
…tions Regenerate package-lock.json for siwe dependency. Add test for past expirationTime rejection. Use suppressExceptions: true in siwe verify() so all errors return as results instead of throwing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The expirationTime coverage is now present, but the lockfile fix did not resolve CI. Both required jobs still fail during npm ci before typecheck or tests can run.
Findings
- [high]
package-lock.jsonremains out of sync withpackage.json/npm resolution. The latest CI run still fails atnpm ciwithMissing: utf-8-validate@5.0.10 from lock filein bothlint-and-typecheckande2e, so the dependency graph is not installable in CI.- File:
package-lock.json:1 - Suggestion: Regenerate the lockfile in a clean checkout using the same npm behavior as CI, verify
npm cisucceeds locally, then commit the resulting lockfile.
- File:
Decision
Request changes. The missing expirationTime test is fixed, but CI is still red on install, so the PR is not mergeable yet.
npm 10 (Node 20, used in CI) requires utf-8-validate@5.0.10 nested entries that npm 11 (Node 24) omits. Regenerated with matching version. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The requested changes are addressed: the lockfile now installs cleanly enough for lint-and-typecheck to pass in CI, and the SIWE tests include the missing past expirationTime rejection. The verifier remains pure and covers domain, URI, chain ID, statement, signature, issuedAt freshness, future-issued messages, expiration, and invalid-message paths.
Findings
- No blocking findings.
Decision
Approve. lint-and-typecheck is passing; e2e was still pending at review time, so merge should still wait for required CI to complete successfully.
Summary
siwe@^2.3.2packagelib/airdrop/siwe-verify.tswithverifySiweRequest(message, signature)— validates full EIP-4361 envelope: signature recovery, domain, URI, chain ID, statement, and issuedAt freshness{ ok: true, address }or{ ok: false, error }— no DB writes (pure verification)Replaces
v1's
lib/airdrop/verify-wallet.tsfor all v5 signed endpoints. v1 file kept for legacy compat.Version
1.30.1 → 1.30.2
🤖 Generated with Claude Code