Production-ready multi-chain batch sender: Base Account + EIP-5792, testable core modules, security & UI polish

gitignore → .gitignore

@@ -1,6 +1,5 @@
 # Dependencies
 node_modules/
-package-lock.json
 yarn.lock
 pnpm-lock.yaml
 

ARCHITECTURE.md

@@ -0,0 +1,143 @@
+# FarSend — Principal Software Architect Review
+
+**Reviewer role:** Principal Software Architect
+**Scope:** Full client-side codebase — `main.js`, `index.html`, `splash.html`, `public/chains.json`, `vite.config.js`, `vercel.json`, `package.json`, `BatchSender.sol`, `README.md`.
+**Branch:** `arena/01a019cb-farsend`
+**Date:** 2026-08-20
+
+This is a structural/architectural review (not a re-run of the security audit — see `AUDIT.md` for that). It focuses on **layering, maintainability, data consistency, testability, resilience, and operational correctness.**
+
+---
+
+## 1. Executive summary
+
+FarSend is a **single-page static dApp**: no backend, no build-time config, all business logic lives in one ~1,500-line module (`main.js`) inside a single closure (`initializeApp()`), with `index.html` rendering the UI and `public/chains.json` holding per-chain contract config. The core *money path* is sound (BigInt math, exact-amount approval, all-or-nothing stateless contract). The architectural risks are **not** in the fund-moving logic — they are in **how the app is structured, how config is sourced, and how state/async flows are managed.**
+
+**Top structural risks, in priority order:**
+
+| # | Risk | Severity | Type |
+|---|------|----------|------|
+| 1 | Monolithic module — no layering, no testability | High | Maintainability |
+| 2 | Chain config has **two sources of truth** (AppKit networks + `chains.json`) that can drift | High | Correctness/Data |
+| 3 | Duplicate `redirects` key in `vercel.json` silently drops config | High | Operational (FIXED) |
+| 4 | Runtime-served config trusted without integrity pinning | Medium | Security/Data |
+| 5 | Async state races (approval checks fire on every keystroke) | Medium | Resilience |
+| 6 | No automated tests or CI for a money-moving app | High | Quality |
+
+---
+
+## 2. Architecture review by concern
+
+### 2.1 Layering & modularity — ❌ (High)
+- **Problem:** `main.js` interleaves five concerns in one closure: (1) config fetch/validation, (2) web3/provider wiring, (3) input parsing/validation, (4) distribution math, (5) DOM/UI rendering + event handling. Everything shares one mutable `state` object and a dozen sibling closures.
+- **Impact:** Any change risks the money path; nothing is unit-testable in isolation; onboarding a second dev is slow; dead code is hard to remove.
+- **Recommended target structure (Vite native, no framework needed):**
+  ```
+  src/
+    config.js          # chains + ABI + constants (single source of truth)
+    web3/chain.js      # provider/signer/chain-switch primitives
+    core/parse.js      # pure: parse CSV/text/JSON -> normalized recipients
+    core/validate.js   # pure: address/burn/decimals/batch-cap checks
+    core/distribute.js # pure: fixed & random distribution math
+    core/dispatch.js   # money-path orchestration (BigInt amounts)
+    ui/notify.js       # notification/toast
+    ui/stepper.js      # step indicator
+    ui/render.js       # DOM updates (summary, preview, warnings)
+    app.js             # composition root
+  ```
+  The pure functions (`parse`, `validate`, `distribute`) have **no DOM or ethers dependency** and are the first things to extract and unit-test.
+
+### 2.2 Single source of truth for chains — ❌ (High)
+- **Problem:** Chain membership is defined **twice**:
+  1. `main.js` imports `[base, mainnet, optimism, arbitrum, bsc, avalanche, polygon]` + a hand-rolled `litvmLiteForge` from `@reown/appkit/networks`.
+  2. `public/chains.json` lists chains with RPC/explorer/contract addresses.
+- **Impact:** Adding a chain means editing **two places**; if they drift (e.g., a chain in `chains.json` but not in AppKit's network list), wallet connection for that chain silently fails or behaves inconsistently. This is exactly the kind of dual-source bug that ships to production.
+- **Recommendation:** Derive AppKit networks from the same config, or validate at build time that the AppKit network set and `chains.json` keys are identical. A tiny script (`scripts/check-chains.mjs`) in CI catches drift.
+
+### 2.3 Runtime config integrity — ⚠️ (Medium)
+- **Problem:** The allowance *spender* and the ETH *destination* both come from whatever `chains.json` is served. There is no allowlist/canary.
+- **Trade-off acknowledged:** the deliberate design (add chains without redeploying) is convenient, but the served config is a single point of trust.
+- **Recommendation (defense-in-depth):** hardcode the canonical Base address as a compiled-in canary and cross-check the fetched config against it on boot; reject the config if it doesn't match. This keeps the "add chain without redeploy" workflow while removing the silent-supply-chain risk.
+
+### 2.4 Async state management — ⚠️ (Medium)
+- **Problem:** `updateSummary()` is `async` and calls `checkAndPromptApproval()` (a live RPC `allowance()` call) on **every** input event and chain change. Fast typing produces interleaved in-flight promises that can leave `dispatchBtn` in a stale state.
+- **Recommendation:** (a) debounce input; (b) use a monotonically increasing **generation token** so only the latest async result is applied; (c) separate "derive enabled/disabled from sync state" from "re-fetch allowance" so the button never flickers while an RPC is in flight.
+
+### 2.5 Resilience / failure handling — ⚠️ (Medium)
+- **Inconsistencies:** some functions return booleans (`validateERC20Address`), some throw (`handleDispatch`), some return early. No central error boundary.
+- **Single RPC URL per chain** — no fallback if a provider is down.
+- **Recommendation:** standardize on throwing domain errors and catching in one place; add a light provider fallback list per chain.
+
+### 2.6 Operational config — ✅ (one bug FIXED)
+- `vercel.json` previously had a **duplicate `redirects` key**, so the Farcaster manifest redirect was silently discarded. **Fixed** — both redirects now live in a single array.
+- Remaining: the `/.well-known/farcaster.json` **static file** and the **redirect** to the hosted manifest both exist; verify which is authoritative in production.
+- Remaining: `farcaster.json` declares `webhookUrl: /api/webhook` but no `/api` endpoint exists in this static deploy — dead config to remove or implement.
+
+### 2.7 Bundle & performance — ⚠️ (Medium)
+- Build warns: `reown` chunk is ~1.3 MB raw / ~400 KB gzip. This is a heavy dependency for a mini-app. Consider dynamic `import()` on first user gesture and review AppKit's tree-shaking.
+- 1.5s chain-check polling remains (safety net); acceptable but keep an eye on it.
+
+### 2.8 Accessibility — ⚠️ (Low→Medium)
+- Emoji-as-icons replaced with proper inline **SVG** icons (`aria-hidden` + `focusable="false"` on decorative ones); notifications are now a live region (`role="status"` + `aria-live="polite"`). Good progress.
+- Remaining: run a Lighthouse/AXE pass; ensure color-only status cues have a text fallback (they do via notification text).
+
+### 2.9 Testing & CI — ✅ tests added / ⚠️ CI not yet active on GitHub
+- Vitest suite now covers the pure logic (`src/core/*`): parsing, validation, distribution, debounce, error decoding, RPC fallback, and the EIP-5792 dispatch layer (see `test/`).
+- The GitHub Action (`.github/workflows/ci.yml`) is written but **not active on GitHub**: the automation account for this branch lacks the `workflows` permission, so the file is held locally until that is granted. It runs `npm ci`, `node --check`, `check:chains`, `npm test`, and `npm run build` — the same pipeline can be run locally with those commands.
+
+---
+
+## 3. What's good (preserve these)
+
+- **BigInt end-to-end on the money path** — amounts go through `ethers.parseUnits` + `BigInt`; no float precision reaches the chain.
+- **Exact-amount approval** — the contract spender allowance equals the required total, minimizing exposure.
+- **All-or-nothing contract** — `BatchSender.sol` is immutable/stateless; any single recipient failure reverts the whole tx, so the sender never loses funds. No owner/withdraw/upgrade.
+- **Pre-flight safety checks** — balance + allowance checks before dispatch, gas estimation with buffer, batch cap, and the burn/dead-address confirmation gate.
+- **Clear UX stepper** (Connect → Define → Dispatch) and defensive reset on disconnect/account/chain change.
+
+---
+
+## 4. Prioritized action plan
+
+**P0 (correctness/ops):**
+1. ✅ Fix `vercel.json` duplicate `redirects` key (done).
+2. ✅ Add build/CI-time chain-config drift guard — `scripts/check-chains.mjs` verifies `chains.json` shape, addresses, unique IDs, and that the chain set matches the AppKit networks imported in `main.js` (single-source-of-truth guard). Runs in CI and via `npm run check:chains`.
+
+**P1 (structure/quality):**
+3. ✅ Extract pure functions into modules + Vitest tests (done):
+   - `src/core/parse.js` — `parseRecipients`, `validateRecipients`, `splitLine`
+   - `src/core/validate.js` — `isBurnAddress`, `findBurnRecipients`, `burnTotal`, `DEFAULT_BURN_ADDRESSES`
+   - `src/core/distribute.js` — `extractAddresses`, `applyFixedAmount`, `generateRandomDistribution`
+   - `test/{parse,validate,distribute}.test.js` — **28 passing tests**
+4. ⚠️ CI — `.github/workflows/ci.yml` is written (`npm ci` → `node --check` → `check:chains` → `npm test` → `npm run build`) and `package-lock.json` is committed so `npm ci` is reproducible, **but the workflow is not active on GitHub**: the automation account used for this branch lacks the `workflows` permission, so the file is held locally until that is granted. Treat CI as "ready, not running".
+5. ⏳ Compiled-in contract-address canary / config integrity check — still open (the drift guard partially covers this; a boot-time canary that rejects a mismatched served config remains).
+
+**P2 (resilience/UX):**
+6. ✅ Debounce + generation-token the async summary/allowance path (done):
+   - `updateSummary()` now bumps a `summaryGeneration` token and abandons stale in-flight results (no interleaving races on `dispatchBtn`/stepper/approval UI).
+   - ERC-20 allowance check split into `computeApproval()` (pure, no DOM) + `applyApprovalUI()` (guarded apply) + thin `checkAndPromptApproval()` for the dispatch path.
+   - High-frequency inputs (recipient textarea, ERC-20 address) are debounced via `src/core/debounce.js` (300ms/250ms), cutting live RPC calls during typing.
+7. ✅ Standardize error handling + provider fallback (done):
+   - `src/core/errors.js` — `describeError()` centralizes user-facing messages (ACTION_REJECTED/4001 → "rejected by user", NETWORK_ERROR/SERVER_ERROR/TIMEOUT → "network request failed", revert-data decoded via `decodeRevertReason()` against the ABI). Refactored connect/approve/dispatch/approval-check error sites to use it.
+   - `src/core/rpc.js` — `getChainRpcUrls()`/`createFallbackProvider()`/`readWithFallback()`: read-only calls (token metadata, allowance, balance checks) fall back to a public RPC if the wallet's RPC is flaky. Signing never uses the fallback. Added `fallbackRpcUrls` to `chains.json` for Base/ETH/OP/Arbitrum/Polygon.
+   - Added `errors` + `rpc` unit tests (50 total now).
+8. ✅ Lazy-load the Reown chunk (done):
+   - Reown AppKit + networks are now a dynamic `import()` booted via `requestIdleCallback` after first paint (fallback: short timeout). Main bundle dropped to ~36KB; the ~1.25MB Reown chunk is code-split and only fetched/parsed when needed. `handleConnect` awaits `ensureAppKit()` so a fast click still works.
+9. ⏸️ Skipped by request — the dead `webhookUrl` in `farcaster.json` was intentionally not addressed (user asked to skip #9).
+
+## Base Account integration (Base App / smart wallet)
+
+FarSend now works with **Base Account** — the passkey-secured ERC-4337 smart wallet powering the Base App:
+
+- **Featured in the wallet modal:** AppKit config sets `featuredWalletIds: [BASE_ACCOUNT_WALLET_ID]` (the official Base Account wallet ID) so it appears first, alongside all other wallets (`allWallets: 'SHOW'`). It connects through the same ethers adapter — no dedicated button; the standard Reown Connect flow is the single entry point for all wallets.
+- **EIP-5792 native dispatch (spec-conformant):** `src/core/sendCalls.js` implements the final EIP-5792 shapes: `wallet_getCapabilities([account, [chainIdHex]])` checking `atomic.status === 'supported' | 'ready'` (per-chain or `0x0` global; legacy draft booleans tolerated, no cross-chain fallback), `wallet_sendCalls` with an **app-provided `id`** (wallet must echo it) returning `{ id }`, and `wallet_getCallsStatus` with **numeric status codes** (1xx pending / 2xx confirmed / 4xx offchain failure / 5xx-6xx onchain failure; legacy strings tolerated).
+- **No-double-send invariant:** once `wallet_sendCalls` *resolves*, the batch is with the wallet and is **never** submitted again — not on poll timeout ("pending in your wallet, do not resubmit"), not on FAILED/UNKNOWN status. The standard `signer.sendTransaction` path is reachable only when `classifySendCallsError()` proves nothing was submitted (capability absent, or a provably pre-submission error such as method-not-found / invalid-params / bundle-too-large — the exact case the spec's Backwards-Compatibility section sanctions). Anything uncertain → abort with instructions to verify in the wallet first. User rejections (4001/4100/5750) are always surfaced.
+- **Unit tested:** `test/sendCalls.test.js` encodes the spec: capability detection (incl. `0x0` handling and the no-cross-chain-fallback rule), batch-id generation, request/response shaping, numeric status mapping, 5730/timeout handling, and the full `classifySendCallsError` decision table.
+
+**Trade-off / note:** gas *sponsorship* for Base Account requires app registration (Base Gasless campaign / paymaster) and is not hardcoded; the SDK path lets a connected smart wallet apply its own gas policies. To add guaranteed sponsored gas later, register the app and pass a `capabilities.paymasterService` on `wallet_sendCalls`.
+
+---
+
+## 5. Bottom line
+
+The **money path is trustworthy** — the contract is stateless and all-or-nothing, and the frontend does BigInt math with pre-flight checks. The **engineering structure is not yet production-grade**: it's a single monolithic module with dual chain-config sources, no tests, and no CI. None of this is a *fund-loss* risk today (that was the audit's finding, now largely addressed), but it will become a *correctness and maintenance* risk as chains and features grow. The highest-leverage work is: **one source of truth for chain config, extract-and-test the pure logic, and add CI.**