Add documentation for implementing wallet additions#17070
Conversation
- Add comprehensive docs/adding-wallets.md guide with: - Image requirements (format, dimensions, naming) - Complete wallet data template with all fields - Supported chain names reference - Language code reference - Common mistakes to avoid - Step-by-step checklist - Update CLAUDE.md with critical requirements section for product additions, emphasizing: - Both assets AND data are required - Two-step implementation process - Chain name verification - Platform flag accuracy This addresses confusion around implementing wallet PRs where agents were omitting required image assets.
minimalsm
requested review from
corwintines,
pettinarip and
wackerow
as code owners
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| 3. **Verify chain names** - When adding `supported_chains`, use exact names from `src/data/chains.ts`. Common mistakes: | ||
| - "Optimism" should be "OP Mainnet" | ||
| - "Polygon" could be "Polygon zkEVM" (chainId 1101) - verify the specific chain |
There was a problem hiding this comment.
This part feels redundant from above, but claude also didn't follow these instructions:
### Type-Safe Chain Names
This project enforces type-safe chain names via TypeScript. When working with layer 2 networks or wallet data:
**Critical Files:**
- `src/data/chains.ts` - Canonical source of all chain names (auto-updated weekly)
- `src/lib/types.ts` - Defines `ChainName` type derived from chains.ts
- `src/data/networks/networks.ts` - Uses `chainName: ChainName`
- `src/data/wallets/wallet-data.ts` - Uses `supported_chains: ChainName[]`
**Rules:**
1. **Always look up exact names** - Before adding `chainName` or `supported_chains`, search `chains.ts` for the exact `name` value
2. **Names are case-sensitive and exact** - e.g., use `"Zircuit Mainnet"` not `"Zircuit"`, use `"OP Mainnet"` not `"Optimism"`
3. **Run type checking** - Use `npx tsc --noEmit` to verify chain names are valid before committing
4. **Non-EVM chains** - For Starknet and other non-EVM chains, use `NonEVMChainName` type
**Common Mistakes:**
- Using informal names: `"Optimism"` should be `"OP Mainnet"`
- Missing "Mainnet" suffix: `"Zircuit"` should be `"Zircuit Mainnet"`
- Wrong casing: `"zksync Mainnet"` should be `"zkSync Mainnet"`I would think having this explcitely laid out in the docs/adding-wallets.md file would be best to avoid repeating ourselves in multiple places (e.g., if anything changes, harder to maintain the instructions)
@minimalsm Would you agree? Definitely not picky which we keep, but within this file it seems best to only keep one of these
| - Browser extension = `chromium: true` and/or `firefox: true` | ||
| - Mobile app = `ios: true` and/or `android: true` | ||
|
|
||
| 5. **Run build verification** - Always run `pnpm build` to catch TypeScript errors before committing |
There was a problem hiding this comment.
pnpm build may be a bulky ask given the weight of our repo:
npx tsc --noEmitThis should be funcitonally equivalent, and only takes a couple seconds to run
| 5. **Run build verification** - Always run `pnpm build` to catch TypeScript errors before committing | |
| 5. **Run TypeScript verification** - Always run `npx tsc --noEmit` to catch TypeScript errors before committing |
| import WalletNameImage from "@/public/images/wallets/walletname.png" | ||
| ``` | ||
|
|
||
| Import statements are ordered alphabetically by variable name. |
There was a problem hiding this comment.
Should we emphasize that this is required for build to pass? npx tsx --noEmit will catch this, so maybe not a big deal
|
|
||
| ### TypeScript Validation | ||
|
|
||
| Run the build to check for type errors: |
There was a problem hiding this comment.
Same as above, would recommend npx tsc --noEmit
| Run the build to check for type errors: | |
| Run the TypeScript analyzer to check for type errors: | |
|
|
||
| Run the build to check for type errors: | ||
| ```bash | ||
| pnpm build |
There was a problem hiding this comment.
| pnpm build | |
| npx tsc --noEmit |
| ``` | ||
| **Correct**: Using exact names from chains.ts | ||
| ```typescript | ||
| supported_chains: ["OP Mainnet", "Polygon zkEVM", "Gnosis"], |
There was a problem hiding this comment.
lol.. assuming the LLM wrote this, it didn't even check chains.ts for proper examples:
| supported_chains: ["OP Mainnet", "Polygon zkEVM", "Gnosis"], | |
| supported_chains: ["OP Mainnet", "Polygon zkEVM"], |
| - [ ] `last_updated` set to today's date | ||
| - [ ] Chain names match exactly with `src/data/chains.ts` | ||
| - [ ] Platform flags accurately reflect wallet availability | ||
| - [ ] `pnpm build` succeeds without errors |
There was a problem hiding this comment.
Similar to above, in particular for LLM jobs, asking it to run pnpm build will add bloat. npx tsc --noEmit should suffice for the vast majority of issues we encounter. If that passes, I'd say Netlify can then do the actual building part.
| - [ ] `pnpm build` succeeds without errors | |
| - [ ] `npx tsc --noEmit` succeeds without errors |
There was a problem hiding this comment.
Perhaps an optional step for any other contributors to actually run a full pnpm build and preview to ensure completion and accuracy.
|
This issue is stale because it has been open 30 days with no activity. |
github-actions
Bot
added
the
Status: Stale
2 participants
Summary
docs/adding-wallets.mdguide for implementing wallet additionsCLAUDE.mdwith critical requirements section for product additionsThis addresses #17069 where AI agents were confidently but incorrectly implementing wallet additions (e.g., omitting required image assets as seen in #17064 vs #17065).
Changes
New:
docs/adding-wallets.mdA complete implementation guide covering:
src/data/chains.tsexactly)Updated:
CLAUDE.mdAdded "Adding Products" section with critical requirements:
Why This Matters
PR #17064 demonstrates a common AI agent failure mode: confidently implementing partial solutions. The agent added wallet data but missed the required image asset. This documentation makes the two-component requirement explicit and provides a checklist to prevent similar issues.
Fixes #17069