-
-
Notifications
You must be signed in to change notification settings - Fork 35
[Refactor] unify create pool validation with zod #90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 5 commits
ac1d7e3
8dc5cf2
9b89559
fe03f38
2eef288
2857a7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,72 @@ | ||||||||||||||||||||||||||||||
| --- | ||||||||||||||||||||||||||||||
| phase: 1 | ||||||||||||||||||||||||||||||
| reviewers: [opencode] | ||||||||||||||||||||||||||||||
| reviewed_at: 2026-04-05T20:33:00Z | ||||||||||||||||||||||||||||||
| plans_reviewed: [Implementation complete - no PLAN.md, reviewing final implementation] | ||||||||||||||||||||||||||||||
| --- | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Cross-AI Plan Review — Phase 1 (Form Validation Consolidation) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ## OpenCode Review | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ### 1. Summary | ||||||||||||||||||||||||||||||
| The implementation achieves the goal of consolidating form validation into Zod schemas with clear step-based validation. However, there are critical issues around Zod version compatibility, some validation gaps, and missed opportunities to leverage more robust address validation. | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ### 2. Strengths | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| - **Good schema organization**: Three step schemas properly isolate validation concerns per form step (`FormData.ts:30-129`, `131-165`, `167-212`) | ||||||||||||||||||||||||||||||
| - **Clean error mapping**: `validateCurrentStep` correctly maps Zod issues to form errors (`CreateFatePool.tsx:179-185`) | ||||||||||||||||||||||||||||||
| - **Appropriate separation**: `validation.ts` correctly handles blockchain-level validation (viem's `isAddress`/`getAddress`) separate from form validation | ||||||||||||||||||||||||||||||
| - **i18n integration**: Using `validationMessages` for localized error messages (`FormData.ts:2,5`) | ||||||||||||||||||||||||||||||
| - **Type inference**: `FormData` correctly inferred via `z.infer` (`FormData.ts:28`) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ### 3. Concerns | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| | Severity | Issue | Location | | ||||||||||||||||||||||||||||||
| |----------|-------|----------| | ||||||||||||||||||||||||||||||
| | **HIGH** | Zod v4 specified but v3 likely installed (package-lock shows v3.22.4/3.25.x) — `safeParse` API differs significantly between versions | `package.json:38` | | ||||||||||||||||||||||||||||||
| | **HIGH** | ETH_ADDRESS_REGEX only checks format, not checksum — vulnerable to case-sensitive address bugs | `FormData.ts:4,53,91,107,121` | | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reconsider the address validation recommendation. The review flags For basic format validation at the form level, a regex check is typically sufficient, especially when the smart contract will perform the authoritative validation. Adding checksum validation in the frontend may be redundant given this architecture. Based on learnings, in the CreateFatePool component, validation for assetId format is handled by the smart contract rather than frontend validation, and the user prefers smart contract validation over duplicating validation logic in the frontend. 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
| | **MEDIUM** | No max length on string fields — potential DoS with excessively long input | `FormData.ts:8-26` | | ||||||||||||||||||||||||||||||
| | **MEDIUM** | `totalFee >= 100` should be `> 100` — allows exactly 100% which is likely unintended | `FormData.ts:205` | | ||||||||||||||||||||||||||||||
| | **MEDIUM** | `Number()` conversion silently produces NaN for non-numeric strings like "abc" — the `Number.isFinite` check handles it but error message is generic | `FormData.ts:68-81` | | ||||||||||||||||||||||||||||||
| | **LOW** | Step schemas use `.pick()` which creates new object type — not a runtime issue but could use stricter typing | `FormData.ts:30-37` | | ||||||||||||||||||||||||||||||
| | **LOW** | No validation for negative initialDeposit upper bound — user could enter extremely large numbers | `FormData.ts:75-80` | | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ### 4. Suggestions | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| 1. **Fix Zod version**: Verify actual installed version and test `safeParse` compatibility, or downgrade to `"zod": "^3.24.0"` for consistency with installed v3 | ||||||||||||||||||||||||||||||
| 2. **Replace regex with viem**: Use viem's `isAddress` from `validation.ts` for robust checksum validation: | ||||||||||||||||||||||||||||||
| ```typescript | ||||||||||||||||||||||||||||||
| import { isAddress } from 'viem'; | ||||||||||||||||||||||||||||||
| // Replace ETH_ADDRESS_REGEX.test() with isAddress() | ||||||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||||||
| 3. **Add field length limits**: | ||||||||||||||||||||||||||||||
| ```typescript | ||||||||||||||||||||||||||||||
| poolName: z.string().trim().min(1).max(100), | ||||||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||||||
| 4. **Fix total fee logic**: Change `totalFee >= 100` to `totalFee > 100` at `FormData.ts:205` | ||||||||||||||||||||||||||||||
| 5. **Add upper bound validation** for initialDeposit (e.g., max 10^18 for ETH denomination) | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Fix markdown formatting around code blocks. The code examples are missing blank lines before and after the fenced code blocks, which violates markdown best practices and triggers linting warnings. 📝 Proposed formatting fixes 1. **Fix Zod version**: Verify actual installed version and test `safeParse` compatibility, or downgrade to `"zod": "^3.24.0"` for consistency with installed v3
2. **Replace regex with viem**: Use viem's `isAddress` from `validation.ts` for robust checksum validation:
+
```typescript
import { isAddress } from 'viem';
// Replace ETH_ADDRESS_REGEX.test() with isAddress()
```
+
3. **Add field length limits**:
+
```typescript
poolName: z.string().trim().min(1).max(100),
```
+
4. **Fix total fee logic**: Change `totalFee >= 100` to `totalFee > 100` at `FormData.ts:205`
5. **Add upper bound validation** for initialDeposit (e.g., max 10^18 for ETH denomination)📝 Committable suggestion
Suggested change
🧰 Tools🪛 markdownlint-cli2 (0.22.0)[warning] 39-39: Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) [warning] 42-42: Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) [warning] 44-44: Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) [warning] 46-46: Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ### 5. Risk Level: **MEDIUM** | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| **Justification**: The Zod version mismatch is the highest risk — if v4 is installed, `safeParse` behavior may differ. The regex-only address validation is a security gap for checksum verification. However, the core refactoring is well-structured, error handling is robust, and the separation of concerns is sound. Fixing the version and address validation issues would reduce risk to LOW. | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| --- | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ## Consensus Summary | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Only one reviewer (OpenCode) available for this review cycle. | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ### Key Action Items | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| 1. **CRITICAL**: Verify Zod version in `package-lock.json` and ensure `safeParse` API compatibility | ||||||||||||||||||||||||||||||
| 2. **HIGH**: Replace `ETH_ADDRESS_REGEX` with viem's `isAddress()` for proper checksum validation | ||||||||||||||||||||||||||||||
| 3. **MEDIUM**: Fix total fee logic (`>= 100` → `> 100`) | ||||||||||||||||||||||||||||||
| 4. **MEDIUM**: Add max length constraints to string fields | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ### Implementation Already Correct | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| - ✅ Zod explicitly added as dependency | ||||||||||||||||||||||||||||||
| - ✅ FormData type inferred via `z.infer` | ||||||||||||||||||||||||||||||
| - ✅ validateCurrentStep refactored to use `.safeParse()` | ||||||||||||||||||||||||||||||
| - ✅ validation.ts retains only blockchain-level validators (no duplicate form validation) | ||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Minor grammar: use hyphen for compound modifier.
"Three step schemas" should be "three-step schemas" when used as a compound modifier before a noun.
🧰 Tools
🪛 LanguageTool
[grammar] ~17-~17: Use a hyphen to join words.
Context: ...s - Good schema organization: Three step schemas properly isolate validation...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents