Fix depositFromBasketCurrency unsupported currency handling and add r…#380
Conversation
|
@efuncode Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughThis PR enhances currency validation in the basket deposit controller. The ChangesCurrency Validation Enhancement
Possibly related PRs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/controllers/mintController.test.ts`:
- Around line 128-148: The test currently expects depositFromBasketCurrency to
call next with an AppError for unsupported currency "JPY", but
depositBodySchema.safeParse fails first and the handler responds with
res.status(400).json(...); update the test to assert the HTTP 400 response
payload (inspect res.status and res.json/mock.calls) for the validation error
instead of reading next.mock.calls, or alternatively move the
unsupported-currency check in depositFromBasketCurrency to run before
depositBodySchema.safeParse so next receives an AppError; refer to
depositFromBasketCurrency, depositBodySchema.safeParse, AppError, and the test
helpers res and next when making the change.
In `@src/controllers/mintController.ts`:
- Around line 265-278: The currency zod schema currently enforces .length(3)
which rejects values like USDC/USDT before your forbidden-vs-allowed logic can
run; remove the .length(3) constraint and keep only normalization/shape rules
(e.g., .string().transform(v => v.toUpperCase())) so safeParse succeeds for 3+
letter codes, then move the allow/forbid decision out of the schema into the
controller flow: after parsing (where safeParse is used) call
depositFromBasketCurrency (or the existing
isAllowedDepositCurrency/isForbiddenDepositCurrency checks) and return the
specific DEPOSIT_ONLY_BASKET_CURRENCIES or INVALID_CURRENCY responses based on
those checks, ensuring BASKET_CURRENCIES and FORBIDDEN_DEPOSIT_CURRENCIES remain
the source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb769657-44aa-4b71-b161-3604a92936ad
📒 Files selected for processing (2)
src/controllers/mintController.test.tssrc/controllers/mintController.ts
| it("rejects unsupported basket deposit currencies with a clear error", async () => { | ||
| const res = makeRes(); | ||
| const next = makeNext(); | ||
| await depositFromBasketCurrency( | ||
| { | ||
| apiKey: { id: "key-1", userId: "user-1", organizationId: null, permissions: [], rateLimit: 100 }, | ||
| body: { | ||
| currency: "JPY", | ||
| amount: "100", | ||
| wallet_address: "GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF", | ||
| }, | ||
| } as unknown as AuthRequest, | ||
| res, | ||
| next, | ||
| ); | ||
|
|
||
| const err = (next as jest.Mock).mock.calls[0][0] as AppError; | ||
| expect(err).toBeInstanceOf(AppError); | ||
| expect(err.statusCode).toBe(400); | ||
| expect(err.message).toContain("currency must be one of"); | ||
| }); |
There was a problem hiding this comment.
This regression test is asserting the wrong failure path.
For "JPY", depositBodySchema.safeParse() fails and depositFromBasketCurrency() responds with res.status(400).json(...); it does not forward an AppError to next. As written, next.mock.calls[0] will be undefined here, so the test won't cover the intended behavior.
Either assert the 400 response payload, or move unsupported-currency handling back below schema validation before expecting next to receive an AppError.
🧰 Tools
🪛 ESLint
[error] 128-128: Delete ␍
(prettier/prettier)
[error] 129-129: Delete ␍
(prettier/prettier)
[error] 130-130: Delete ␍
(prettier/prettier)
[error] 131-131: Delete ␍
(prettier/prettier)
[error] 132-132: Delete ␍
(prettier/prettier)
[error] 133-133: Replace ·id:·"key-1",·userId:·"user-1",·organizationId:·null,·permissions:·[],·rateLimit:·100·},␍ with ⏎··········id:·"key-1",⏎··········userId:·"user-1",⏎··········organizationId:·null,⏎··········permissions:·[],⏎··········rateLimit:·100,⏎········},
(prettier/prettier)
[error] 134-134: Delete ␍
(prettier/prettier)
[error] 135-135: Delete ␍
(prettier/prettier)
[error] 136-136: Delete ␍
(prettier/prettier)
[error] 137-137: Delete ␍
(prettier/prettier)
[error] 138-138: Delete ␍
(prettier/prettier)
[error] 139-139: Delete ␍
(prettier/prettier)
[error] 140-140: Delete ␍
(prettier/prettier)
[error] 141-141: Delete ␍
(prettier/prettier)
[error] 142-142: Delete ␍
(prettier/prettier)
[error] 143-143: Delete ␍
(prettier/prettier)
[error] 144-144: Delete ␍
(prettier/prettier)
[error] 145-145: Delete ␍
(prettier/prettier)
[error] 146-146: Delete ␍
(prettier/prettier)
[error] 147-147: Delete ␍
(prettier/prettier)
[error] 148-148: Delete ␍
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/controllers/mintController.test.ts` around lines 128 - 148, The test
currently expects depositFromBasketCurrency to call next with an AppError for
unsupported currency "JPY", but depositBodySchema.safeParse fails first and the
handler responds with res.status(400).json(...); update the test to assert the
HTTP 400 response payload (inspect res.status and res.json/mock.calls) for the
validation error instead of reading next.mock.calls, or alternatively move the
unsupported-currency check in depositFromBasketCurrency to run before
depositBodySchema.safeParse so next receives an AppError; refer to
depositFromBasketCurrency, depositBodySchema.safeParse, AppError, and the test
helpers res and next when making the change.
| currency: z | ||
| .string() | ||
| .length(3) | ||
| .transform((value) => value.toUpperCase()) | ||
| .refine( | ||
| (currency) => | ||
| isAllowedDepositCurrency(currency) || isForbiddenDepositCurrency(currency), | ||
| { | ||
| message: `Currency must be one of: ${[ | ||
| ...BASKET_CURRENCIES, | ||
| ...FORBIDDEN_DEPOSIT_CURRENCIES, | ||
| ].join(", ")}`, | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Don't collapse unsupported and forbidden currencies into the generic schema error path.
z.string().length(3) rejects USDC/USDT before the forbidden-currency branch can run, and unsupported 3-letter codes like JPY now stop at safeParse() and return the generic "Invalid request" response from Lines 299-304. That makes the explicit deposit errors at Lines 307-321 unreachable for the cases this PR is trying to improve.
If you want the controller to return the clearer DEPOSIT_ONLY_BASKET_CURRENCIES / INVALID_CURRENCY responses, keep this schema focused on normalization/shape validation and leave the allow-vs-forbid decision to depositFromBasketCurrency.
🧰 Tools
🪛 ESLint
[error] 270-271: Delete ⏎·······
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/controllers/mintController.ts` around lines 265 - 278, The currency zod
schema currently enforces .length(3) which rejects values like USDC/USDT before
your forbidden-vs-allowed logic can run; remove the .length(3) constraint and
keep only normalization/shape rules (e.g., .string().transform(v =>
v.toUpperCase())) so safeParse succeeds for 3+ letter codes, then move the
allow/forbid decision out of the schema into the controller flow: after parsing
(where safeParse is used) call depositFromBasketCurrency (or the existing
isAllowedDepositCurrency/isForbiddenDepositCurrency checks) and return the
specific DEPOSIT_ONLY_BASKET_CURRENCIES or INVALID_CURRENCY responses based on
those checks, ensuring BASKET_CURRENCIES and FORBIDDEN_DEPOSIT_CURRENCIES remain
the source of truth.
2 participants
close #281
Summary by CodeRabbit
Bug Fixes
Tests