From c4ada9978383d076c3a27849d7a5cd271f675e19 Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 6 Mar 2024 11:29:46 +0100 Subject: [PATCH 1/6] Improve `ZodErrorFilter` to only return first union error --- .../common/filters/zod-error.filter.spec.ts | 144 ++++++++++++++++++ src/routes/common/filters/zod-error.filter.ts | 12 +- 2 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 src/routes/common/filters/zod-error.filter.spec.ts diff --git a/src/routes/common/filters/zod-error.filter.spec.ts b/src/routes/common/filters/zod-error.filter.spec.ts new file mode 100644 index 0000000000..a15a221f6a --- /dev/null +++ b/src/routes/common/filters/zod-error.filter.spec.ts @@ -0,0 +1,144 @@ +import { Body, Controller, Get, INestApplication, Post } from '@nestjs/common'; +import { Test, TestingModule } from '@nestjs/testing'; +import { TestAppProvider } from '@/__tests__/test-app.provider'; +import * as request from 'supertest'; +import { APP_FILTER } from '@nestjs/core'; +import { TestLoggingModule } from '@/logging/__tests__/test.logging.module'; +import { ConfigurationModule } from '@/config/configuration.module'; +import configuration from '@/config/entities/__tests__/configuration'; +import { ZodErrorFilter } from '@/routes/common/filters/zod-error.filter'; +import { z } from 'zod'; +import { ValidationPipe } from '@/validation/pipes/validation.pipe'; +import { faker } from '@faker-js/faker'; + +const ZodSchema = z.object({ + value: z.string(), + secondValue: z.string(), +}); + +const ZodUnionSchema = z.union([ + ZodSchema, + z.object({ + secondUnionValue: z.number(), + secondUnionSecondValue: z.number(), + }), +]); + +const ZodNestedUnionSchema = z.union([ + z.object({ first: ZodUnionSchema }), + z.object({ second: ZodUnionSchema }), +]); + +@Controller({}) +class TestController { + @Post('zod-exception') + async zodError( + @Body(new ValidationPipe(ZodSchema)) body: z.infer, + ): Promise> { + return body; + } + + @Post('zod-union-exception') + async zodUnionError( + @Body(new ValidationPipe(ZodUnionSchema)) + body: z.infer, + ): Promise> { + return body; + } + + @Post('zod-nested-union-exception') + async zodNestedUnionError( + @Body(new ValidationPipe(ZodNestedUnionSchema)) + body: z.infer, + ): Promise> { + return body; + } + + @Get('non-zod-exception') + async nonZodException(): Promise { + throw new Error('Some random error'); + } +} + +describe('ZodErrorFilter tests', () => { + let app: INestApplication; + + beforeEach(async () => { + const moduleFixture: TestingModule = await Test.createTestingModule({ + imports: [TestLoggingModule, ConfigurationModule.register(configuration)], + controllers: [TestController], + providers: [ + { + provide: APP_FILTER, + useClass: ZodErrorFilter, + }, + ], + }).compile(); + + app = await new TestAppProvider().provide(moduleFixture); + await app.init(); + }); + + afterEach(async () => { + await app.close(); + }); + + it('ZodError exception returns first issue', async () => { + await request(app.getHttpServer()) + .post('/zod-exception') + .send({ value: faker.number.int() }) + .expect(422) + .expect({ + statusCode: 422, + code: 'invalid_type', + expected: 'string', + received: 'number', + path: ['value'], // Only the first issue is returned + message: 'Expected string, received number', + }); + }); + + it('ZodError union exception returns first issue of first union issue', async () => { + await request(app.getHttpServer()) + .post('/zod-union-exception') + .send({ value: faker.datatype.boolean() }) + .expect(422) + .expect({ + statusCode: 422, + code: 'invalid_type', + expected: 'string', + received: 'boolean', + path: ['value'], // Only the first union, first issue is returned + message: 'Expected string, received boolean', + }); + }); + + it('ZodError nested union exception returns first issue of nested union issue', async () => { + await request(app.getHttpServer()) + .post('/zod-union-exception') + .send({ + first: { + secondUnionValue: faker.datatype.boolean(), + }, + }) + .expect(422) + .expect({ + statusCode: 422, + code: 'invalid_type', + expected: 'string', + received: 'undefined', + path: ['value'], // Only the first union of the nested union error, first issue is returned + message: 'Required', + }); + }); + + it('non-ZodError exception returns correct error code and message', async () => { + await request(app.getHttpServer()) + .get('/non-zod-exception') + .expect(500) + .expect({ + statusCode: 500, + message: 'Internal server error', + }); + }); +}); diff --git a/src/routes/common/filters/zod-error.filter.ts b/src/routes/common/filters/zod-error.filter.ts index 2dbd5c2e2a..d34cd792fe 100644 --- a/src/routes/common/filters/zod-error.filter.ts +++ b/src/routes/common/filters/zod-error.filter.ts @@ -4,7 +4,7 @@ import { ExceptionFilter, HttpStatus, } from '@nestjs/common'; -import { ZodError } from 'zod'; +import { ZodError, ZodIssue } from 'zod'; import { Response } from 'express'; /** @@ -20,9 +20,17 @@ export class ZodErrorFilter implements ExceptionFilter { const ctx = host.switchToHttp(); const response = ctx.getResponse(); + const error = this.mapZodErrorResponse(exception); + response.status(HttpStatus.UNPROCESSABLE_ENTITY).json({ statusCode: HttpStatus.UNPROCESSABLE_ENTITY, - ...exception.issues[0], + ...error, }); } + + private mapZodErrorResponse(exception: ZodError): ZodIssue { + return exception.issues[0].code === 'invalid_union' + ? this.mapZodErrorResponse(exception.issues[0].unionErrors[0]) + : exception.issues[0]; + } } From 9f26207e1716f3b23a853f4b6b8c024b6d6c0b88 Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 6 Mar 2024 11:34:51 +0100 Subject: [PATCH 2/6] Migrate `Balance` validation to `zod` --- .../balances-api/safe-balances-api.service.ts | 2 +- .../zerion-balances-api.service.ts | 3 +- src/domain.module.ts | 2 - src/domain/balances/balances.repository.ts | 5 +- src/domain/balances/balances.validator.ts | 36 ------------ .../entities/__tests__/balance.builder.ts | 3 +- .../balances/entities/balance.entity.ts | 24 +++----- .../balances/entities/balance.token.entity.ts | 10 ++-- .../entities/schemas/balance.schema.ts | 56 +++++++++---------- .../zerion-balances.controller.spec.ts | 13 ++++- .../balances/balances.controller.spec.ts | 46 +++++++++------ 11 files changed, 87 insertions(+), 113 deletions(-) delete mode 100644 src/domain/balances/balances.validator.ts diff --git a/src/datasources/balances-api/safe-balances-api.service.ts b/src/datasources/balances-api/safe-balances-api.service.ts index 1de0bcd920..a8403e7895 100644 --- a/src/datasources/balances-api/safe-balances-api.service.ts +++ b/src/datasources/balances-api/safe-balances-api.service.ts @@ -125,7 +125,7 @@ export class SafeBalancesApi implements IBalancesApi { ): Promise { const tokenAddresses = balances .map((balance) => balance.tokenAddress) - .filter((address): address is string => address !== null); + .filter((address): address is `0x${string}` => address !== null); const assetPrices = await this.coingeckoApi.getTokenPrices({ chainId: this.chainId, diff --git a/src/datasources/balances-api/zerion-balances-api.service.ts b/src/datasources/balances-api/zerion-balances-api.service.ts index 251cbde668..75614b8496 100644 --- a/src/datasources/balances-api/zerion-balances-api.service.ts +++ b/src/datasources/balances-api/zerion-balances-api.service.ts @@ -32,6 +32,7 @@ import { DataSourceError } from '@/domain/errors/data-source.error'; import { IBalancesApi } from '@/domain/interfaces/balances-api.interface'; import { ILoggingService, LoggingService } from '@/logging/logging.interface'; import { Inject, Injectable } from '@nestjs/common'; +import { getAddress } from 'viem'; export const IZerionBalancesApi = Symbol('IZerionBalancesApi'); @@ -230,7 +231,7 @@ export class ZerionBalancesApi implements IBalancesApi { ): Erc20Balance { const { fungible_info, quantity } = zerionBalanceAttributes; return { - tokenAddress, + tokenAddress: getAddress(tokenAddress), token: { name: fungible_info.name ?? '', symbol: fungible_info.symbol ?? '', diff --git a/src/domain.module.ts b/src/domain.module.ts index 7382893aed..457e60d073 100644 --- a/src/domain.module.ts +++ b/src/domain.module.ts @@ -50,7 +50,6 @@ import { HealthRepository } from '@/domain/health/health.repository'; import { HumanDescriptionApiModule } from '@/datasources/human-description-api/human-description-api.module'; import { IHumanDescriptionRepository } from '@/domain/human-description/human-description.repository.interface'; import { HumanDescriptionRepository } from '@/domain/human-description/human-description.repository'; -import { BalancesValidator } from '@/domain/balances/balances.validator'; import { BalancesApiModule } from '@/datasources/balances-api/balances-api.module'; @Global() @@ -95,7 +94,6 @@ import { BalancesApiModule } from '@/datasources/balances-api/balances-api.modul SafeAppsValidator, SafeListValidator, SafeValidator, - BalancesValidator, TokenValidator, TransactionTypeValidator, TransferValidator, diff --git a/src/domain/balances/balances.repository.ts b/src/domain/balances/balances.repository.ts index 5782168c48..220a711905 100644 --- a/src/domain/balances/balances.repository.ts +++ b/src/domain/balances/balances.repository.ts @@ -1,7 +1,7 @@ import { Inject, Injectable } from '@nestjs/common'; import { IBalancesRepository } from '@/domain/balances/balances.repository.interface'; import { Balance } from '@/domain/balances/entities/balance.entity'; -import { BalancesValidator } from '@/domain/balances/balances.validator'; +import { BalanceSchema } from '@/domain/balances/entities/schemas/balance.schema'; import { IBalancesApiManager } from '@/domain/interfaces/balances-api.manager.interface'; @Injectable() @@ -9,7 +9,6 @@ export class BalancesRepository implements IBalancesRepository { constructor( @Inject(IBalancesApiManager) private readonly balancesApiManager: IBalancesApiManager, - private readonly balancesValidator: BalancesValidator, ) {} async getBalances(args: { @@ -21,7 +20,7 @@ export class BalancesRepository implements IBalancesRepository { }): Promise { const api = await this.balancesApiManager.getBalancesApi(args.chainId); const balances = await api.getBalances(args); - return balances.map((balance) => this.balancesValidator.validate(balance)); + return balances.map((balance) => BalanceSchema.parse(balance)); } async clearBalances(args: { diff --git a/src/domain/balances/balances.validator.ts b/src/domain/balances/balances.validator.ts deleted file mode 100644 index 3a57f3a503..0000000000 --- a/src/domain/balances/balances.validator.ts +++ /dev/null @@ -1,36 +0,0 @@ -import { ValidateFunction } from 'ajv'; -import { Injectable } from '@nestjs/common'; -import { Balance } from '@/domain/balances/entities/balance.entity'; -import { - BALANCE_SCHEMA_ID, - BALANCE_TOKEN_SCHEMA_ID, - balanceSchema, - balanceTokenSchema, -} from '@/domain/balances/entities/schemas/balance.schema'; -import { IValidator } from '@/domain/interfaces/validator.interface'; -import { GenericValidator } from '@/validation/providers/generic.validator'; -import { JsonSchemaService } from '@/validation/providers/json-schema.service'; - -@Injectable() -export class BalancesValidator implements IValidator { - private readonly isValidBalance: ValidateFunction; - - constructor( - private readonly genericValidator: GenericValidator, - private readonly jsonSchemaService: JsonSchemaService, - ) { - this.jsonSchemaService.getSchema( - BALANCE_TOKEN_SCHEMA_ID, - balanceTokenSchema, - ); - - this.isValidBalance = this.jsonSchemaService.getSchema( - BALANCE_SCHEMA_ID, - balanceSchema, - ); - } - - validate(data: unknown): Balance { - return this.genericValidator.validate(this.isValidBalance, data); - } -} diff --git a/src/domain/balances/entities/__tests__/balance.builder.ts b/src/domain/balances/entities/__tests__/balance.builder.ts index bacb4612fc..a805637199 100644 --- a/src/domain/balances/entities/__tests__/balance.builder.ts +++ b/src/domain/balances/entities/__tests__/balance.builder.ts @@ -2,10 +2,11 @@ import { Builder, IBuilder } from '@/__tests__/builder'; import { faker } from '@faker-js/faker'; import { balanceTokenBuilder } from './balance.token.builder'; import { Balance } from '@/domain/balances/entities/balance.entity'; +import { getAddress } from 'viem'; export function balanceBuilder(): IBuilder { return new Builder() - .with('tokenAddress', faker.finance.ethereumAddress()) + .with('tokenAddress', getAddress(faker.finance.ethereumAddress())) .with('token', balanceTokenBuilder().build()) .with('balance', faker.string.numeric()); } diff --git a/src/domain/balances/entities/balance.entity.ts b/src/domain/balances/entities/balance.entity.ts index 84214469f8..1ae7d36b40 100644 --- a/src/domain/balances/entities/balance.entity.ts +++ b/src/domain/balances/entities/balance.entity.ts @@ -1,18 +1,12 @@ -import { BalanceToken } from '@/domain/balances/entities/balance.token.entity'; +import { + BalanceSchema, + Erc20BalanceSchema, + NativeBalanceSchema, +} from '@/domain/balances/entities/schemas/balance.schema'; +import { z } from 'zod'; -export interface NativeBalance { - tokenAddress: null; - token: null; - balance: string; -} +export type NativeBalance = z.infer; -export interface Erc20Balance { - tokenAddress: string; - token: BalanceToken; - balance: string; -} +export type Erc20Balance = z.infer; -export type Balance = (NativeBalance | Erc20Balance) & { - fiatBalance: string | null; - fiatConversion: string | null; -}; +export type Balance = z.infer; diff --git a/src/domain/balances/entities/balance.token.entity.ts b/src/domain/balances/entities/balance.token.entity.ts index 44ebe0dfe4..2fd53bb7b0 100644 --- a/src/domain/balances/entities/balance.token.entity.ts +++ b/src/domain/balances/entities/balance.token.entity.ts @@ -1,6 +1,4 @@ -export interface BalanceToken { - name: string; - symbol: string; - decimals: number; - logoUri: string; -} +import { BalanceTokenSchema } from '@/domain/balances/entities/schemas/balance.schema'; +import { z } from 'zod'; + +export type BalanceToken = z.infer; diff --git a/src/domain/balances/entities/schemas/balance.schema.ts b/src/domain/balances/entities/schemas/balance.schema.ts index 49bd225f53..3e58c60deb 100644 --- a/src/domain/balances/entities/schemas/balance.schema.ts +++ b/src/domain/balances/entities/schemas/balance.schema.ts @@ -1,35 +1,31 @@ -import { JSONSchemaType, Schema } from 'ajv'; -import { BalanceToken } from '@/domain/balances/entities/balance.token.entity'; +import { z } from 'zod'; +import { AddressSchema } from '@/validation/entities/schemas/address.schema'; -export const BALANCE_TOKEN_SCHEMA_ID = - 'https://safe-client.safe.global/schemas/balances/balance-token.json'; +export const NativeBalanceSchema = z.object({ + tokenAddress: z.null(), + token: z.null(), + balance: z.string(), +}); -const balanceTokenSchema: JSONSchemaType = { - $id: BALANCE_TOKEN_SCHEMA_ID, - type: 'object', - properties: { - name: { type: 'string' }, - symbol: { type: 'string' }, - decimals: { type: 'number' }, - logoUri: { type: 'string' }, - }, - required: ['name', 'symbol', 'decimals', 'logoUri'], -}; +export const BalanceTokenSchema = z.object({ + name: z.string(), + symbol: z.string(), + decimals: z.number(), + logoUri: z.string(), +}); -export const BALANCE_SCHEMA_ID = - 'https://safe-client.safe.global/schemas/balances/balance.json'; +export const Erc20BalanceSchema = z.object({ + tokenAddress: AddressSchema, + token: BalanceTokenSchema, + balance: z.string(), +}); -const balanceSchema: Schema = { - $id: BALANCE_SCHEMA_ID, - type: 'object', - properties: { - tokenAddress: { type: 'string', nullable: true, default: null }, - token: { anyOf: [{ type: 'null' }, { $ref: 'balance-token.json' }] }, - balance: { type: 'string' }, - fiatBalance: { type: 'string', nullable: true, default: null }, - fiatConversion: { type: 'string', nullable: true, default: null }, - }, - required: ['balance'], -}; +const FiatSchema = z.object({ + fiatBalance: z.string().nullable(), + fiatConversion: z.string().nullable(), +}); -export { balanceSchema, balanceTokenSchema }; +export const BalanceSchema = z.union([ + NativeBalanceSchema.merge(FiatSchema), + Erc20BalanceSchema.merge(FiatSchema), +]); diff --git a/src/routes/balances/__tests__/controllers/zerion-balances.controller.spec.ts b/src/routes/balances/__tests__/controllers/zerion-balances.controller.spec.ts index fc18eabef0..2682dffe31 100644 --- a/src/routes/balances/__tests__/controllers/zerion-balances.controller.spec.ts +++ b/src/routes/balances/__tests__/controllers/zerion-balances.controller.spec.ts @@ -30,6 +30,7 @@ import { zerionFlagsBuilder, zerionBalancesBuilder, } from '@/datasources/balances-api/entities/__tests__/zerion-balance.entity.builder'; +import { getAddress } from 'viem'; describe('Balances Controller (Unit)', () => { let app: INestApplication; @@ -202,7 +203,11 @@ describe('Balances Controller (Unit)', () => { { tokenInfo: { type: 'ERC20', - address: erc20TokenFungibleInfo.implementations[0].address, + address: erc20TokenFungibleInfo.implementations[0].address + ? getAddress( + erc20TokenFungibleInfo.implementations[0].address, + ) + : erc20TokenFungibleInfo.implementations[0].address, decimals: 15, symbol: erc20TokenFungibleInfo.symbol, name: erc20TokenFungibleInfo.name, @@ -345,7 +350,11 @@ describe('Balances Controller (Unit)', () => { { tokenInfo: { type: 'ERC20', - address: erc20TokenFungibleInfo.implementations[0].address, + address: erc20TokenFungibleInfo.implementations[0].address + ? getAddress( + erc20TokenFungibleInfo.implementations[0].address, + ) + : erc20TokenFungibleInfo.implementations[0].address, decimals: 15, symbol: erc20TokenFungibleInfo.symbol, name: erc20TokenFungibleInfo.name, diff --git a/src/routes/balances/balances.controller.spec.ts b/src/routes/balances/balances.controller.spec.ts index a5713921d6..5424298a22 100644 --- a/src/routes/balances/balances.controller.spec.ts +++ b/src/routes/balances/balances.controller.spec.ts @@ -23,6 +23,7 @@ import { balanceTokenBuilder } from '@/domain/balances/entities/__tests__/balanc import { NetworkResponseError } from '@/datasources/network/entities/network.error.entity'; import { AccountDataSourceModule } from '@/datasources/account/account.datasource.module'; import { TestAccountDataSourceModule } from '@/datasources/account/__tests__/test.account.datasource.module'; +import { getAddress } from 'viem'; describe('Balances Controller (Unit)', () => { let app: INestApplication; @@ -74,12 +75,12 @@ describe('Balances Controller (Unit)', () => { .with('token', null) .build(), balanceBuilder() - .with('tokenAddress', tokenAddress) + .with('tokenAddress', getAddress(tokenAddress)) .with('balance', '4000000000000000000') .with('token', balanceTokenBuilder().with('decimals', 17).build()) .build(), balanceBuilder() - .with('tokenAddress', secondTokenAddress) + .with('tokenAddress', getAddress(secondTokenAddress)) .with('balance', '3000000000000000000') .with('token', balanceTokenBuilder().with('decimals', 17).build()) .build(), @@ -155,7 +156,9 @@ describe('Balances Controller (Unit)', () => { { tokenInfo: { type: 'ERC20', - address: transactionApiBalancesResponse[1].tokenAddress, + address: transactionApiBalancesResponse[1].tokenAddress + ? getAddress(transactionApiBalancesResponse[1].tokenAddress) + : transactionApiBalancesResponse[1].tokenAddress, decimals: 17, symbol: transactionApiBalancesResponse[1].token?.symbol, name: transactionApiBalancesResponse[1].token?.name, @@ -168,7 +171,9 @@ describe('Balances Controller (Unit)', () => { { tokenInfo: { type: 'ERC20', - address: transactionApiBalancesResponse[2].tokenAddress, + address: transactionApiBalancesResponse[2].tokenAddress + ? getAddress(transactionApiBalancesResponse[2].tokenAddress) + : transactionApiBalancesResponse[2].tokenAddress, decimals: 17, symbol: transactionApiBalancesResponse[2].token?.symbol, name: transactionApiBalancesResponse[2].token?.name, @@ -221,7 +226,7 @@ describe('Balances Controller (Unit)', () => { const tokenAddress = faker.finance.ethereumAddress(); const transactionApiBalancesResponse = [ balanceBuilder() - .with('tokenAddress', tokenAddress) + .with('tokenAddress', getAddress(tokenAddress)) .with('balance', '4000000000000000000') .with('token', balanceTokenBuilder().with('decimals', 17).build()) .build(), @@ -342,7 +347,7 @@ describe('Balances Controller (Unit)', () => { const tokenAddress = faker.finance.ethereumAddress(); const transactionApiBalancesResponse = [ balanceBuilder() - .with('tokenAddress', tokenAddress) + .with('tokenAddress', getAddress(tokenAddress)) .with('balance', '40000000000000000000000000000000000') .with('token', balanceTokenBuilder().with('decimals', 17).build()) .build(), @@ -386,7 +391,9 @@ describe('Balances Controller (Unit)', () => { { tokenInfo: { type: 'ERC20', - address: transactionApiBalancesResponse[0].tokenAddress, + address: transactionApiBalancesResponse[0].tokenAddress + ? getAddress(transactionApiBalancesResponse[0].tokenAddress) + : transactionApiBalancesResponse[0].tokenAddress, decimals: 17, symbol: transactionApiBalancesResponse[0].token?.symbol, name: transactionApiBalancesResponse[0].token?.name, @@ -449,7 +456,7 @@ describe('Balances Controller (Unit)', () => { const tokenAddress = faker.finance.ethereumAddress(); const transactionApiBalancesResponse = [ balanceBuilder() - .with('tokenAddress', tokenAddress) + .with('tokenAddress', getAddress(tokenAddress)) .with('balance', '40000000000000000000000000000000000') .with('token', balanceTokenBuilder().with('decimals', 17).build()) .build(), @@ -488,7 +495,9 @@ describe('Balances Controller (Unit)', () => { { tokenInfo: { type: 'ERC20', - address: transactionApiBalancesResponse[0].tokenAddress, + address: transactionApiBalancesResponse[0].tokenAddress + ? getAddress(transactionApiBalancesResponse[0].tokenAddress) + : transactionApiBalancesResponse[0].tokenAddress, decimals: 17, symbol: transactionApiBalancesResponse[0].token?.symbol, name: transactionApiBalancesResponse[0].token?.name, @@ -507,7 +516,7 @@ describe('Balances Controller (Unit)', () => { it(`should return a 0-balance when a validation error happens`, async () => { const chain = chainBuilder().with('chainId', '10').build(); const safeAddress = faker.finance.ethereumAddress(); - const tokenAddress = faker.finance.ethereumAddress(); + const tokenAddress = getAddress(faker.finance.ethereumAddress()); const transactionApiBalancesResponse = [ balanceBuilder() .with('tokenAddress', tokenAddress) @@ -553,7 +562,9 @@ describe('Balances Controller (Unit)', () => { { tokenInfo: { type: 'ERC20', - address: transactionApiBalancesResponse[0].tokenAddress, + address: transactionApiBalancesResponse[0].tokenAddress + ? getAddress(transactionApiBalancesResponse[0].tokenAddress) + : transactionApiBalancesResponse[0].tokenAddress, decimals: 17, symbol: transactionApiBalancesResponse[0].token?.symbol, name: transactionApiBalancesResponse[0].token?.name, @@ -604,7 +615,7 @@ describe('Balances Controller (Unit)', () => { }); }); - it(`500 error if validation fails`, async () => { + it(`422 error if validation fails`, async () => { const chainId = '1'; const safeAddress = faker.finance.ethereumAddress(); const chainResponse = chainBuilder().with('chainId', chainId).build(); @@ -626,11 +637,14 @@ describe('Balances Controller (Unit)', () => { await request(app.getHttpServer()) .get(`/v1/chains/${chainId}/safes/${safeAddress}/balances/usd`) - .expect(500) + .expect(422) .expect({ - message: 'Validation failed', - code: 42, - arguments: [], + statusCode: 422, + code: 'invalid_type', + expected: 'null', + message: 'Required', + path: ['tokenAddress'], + received: 'undefined', }); expect(networkService.get.mock.calls.length).toBe(3); From 100a81140feac363317bbb23063fdc9386ec3596 Mon Sep 17 00:00:00 2001 From: iamacook Date: Thu, 7 Mar 2024 10:19:38 +0100 Subject: [PATCH 3/6] Allow optional values and add `null` default --- src/domain/balances/entities/schemas/balance.schema.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/domain/balances/entities/schemas/balance.schema.ts b/src/domain/balances/entities/schemas/balance.schema.ts index 3e58c60deb..8589effc37 100644 --- a/src/domain/balances/entities/schemas/balance.schema.ts +++ b/src/domain/balances/entities/schemas/balance.schema.ts @@ -21,8 +21,8 @@ export const Erc20BalanceSchema = z.object({ }); const FiatSchema = z.object({ - fiatBalance: z.string().nullable(), - fiatConversion: z.string().nullable(), + fiatBalance: z.string().optional().nullable().default(null), + fiatConversion: z.string().optional().nullable().default(null), }); export const BalanceSchema = z.union([ From fcb5b9be7fa93ea9638b0d3c76f755f2db1d1f2b Mon Sep 17 00:00:00 2001 From: iamacook Date: Thu, 7 Mar 2024 12:00:05 +0100 Subject: [PATCH 4/6] Add test --- .../schemas/__tests__/balance.schema.spec.ts | 287 ++++++++++++++++++ .../entities/schemas/balance.schema.ts | 5 +- .../balances/balances.controller.spec.ts | 6 +- 3 files changed, 293 insertions(+), 5 deletions(-) create mode 100644 src/domain/balances/entities/schemas/__tests__/balance.schema.spec.ts diff --git a/src/domain/balances/entities/schemas/__tests__/balance.schema.spec.ts b/src/domain/balances/entities/schemas/__tests__/balance.schema.spec.ts new file mode 100644 index 0000000000..95660ef1dd --- /dev/null +++ b/src/domain/balances/entities/schemas/__tests__/balance.schema.spec.ts @@ -0,0 +1,287 @@ +import { balanceBuilder } from '@/domain/balances/entities/__tests__/balance.builder'; +import { balanceTokenBuilder } from '@/domain/balances/entities/__tests__/balance.token.builder'; +import { + BalanceSchema, + BalanceTokenSchema, + Erc20BalanceSchema, + NativeBalanceSchema, +} from '@/domain/balances/entities/schemas/balance.schema'; +import { faker } from '@faker-js/faker'; +import { getAddress } from 'viem'; +import { ZodError } from 'zod'; + +describe('Balances schema', () => { + describe('NativeBalanceSchema', () => { + it('should validate a valid native balance', () => { + const nativeBalance = balanceBuilder() + .with('tokenAddress', null) + .with('token', null) + .build(); + + const result = NativeBalanceSchema.safeParse(nativeBalance); + + expect(result.success).toBe(true); + }); + + it('should allow optional tokenAddress and token and default to null', () => { + const nativeBalance = balanceBuilder().build(); + // @ts-expect-error - inferred types don't allow optional fields + delete nativeBalance.tokenAddress; + // @ts-expect-error - inferred types don't allow optional fields + delete nativeBalance.token; + + const result = NativeBalanceSchema.safeParse(nativeBalance); + + expect(result.success && result.data.tokenAddress).toBe(null); + expect(result.success && result.data.token).toBe(null); + }); + + it('should not allow an invalid native balance', () => { + const nativeBalance = { invalid: 'nativeBalance' }; + + const result = NativeBalanceSchema.safeParse(nativeBalance); + + expect(!result.success && result.error).toStrictEqual( + new ZodError([ + { + code: 'invalid_type', + expected: 'string', + received: 'undefined', + path: ['balance'], + message: 'Required', + }, + ]), + ); + }); + }); + + describe('BalanceTokenSchema', () => { + it('should validate a valid balance token', () => { + const balanceToken = balanceTokenBuilder().build(); + + const result = BalanceTokenSchema.safeParse(balanceToken); + + expect(result.success).toBe(true); + }); + + it('should not allow an invalid balance token', () => { + const balanceToken = { invalid: 'balanceToken' }; + + const result = BalanceTokenSchema.safeParse(balanceToken); + + expect(!result.success && result.error).toStrictEqual( + new ZodError([ + { + code: 'invalid_type', + expected: 'string', + received: 'undefined', + path: ['name'], + message: 'Required', + }, + { + code: 'invalid_type', + expected: 'string', + received: 'undefined', + path: ['symbol'], + message: 'Required', + }, + { + code: 'invalid_type', + expected: 'number', + received: 'undefined', + path: ['decimals'], + message: 'Required', + }, + { + code: 'invalid_type', + expected: 'string', + received: 'undefined', + path: ['logoUri'], + message: 'Required', + }, + ]), + ); + }); + }); + + describe('Erc20BalanceSchema', () => { + it('should validate a valid ERC-20 balance', () => { + const erc20Balance = balanceBuilder().build(); + + const result = Erc20BalanceSchema.safeParse(erc20Balance); + + expect(result.success).toBe(true); + }); + + it('should checksum the tokenAddress', () => { + const nonChecksummedTokenAddress = faker.finance + .ethereumAddress() + .toLowerCase() as `0x${string}`; + const erc20Balance = balanceBuilder() + .with('tokenAddress', nonChecksummedTokenAddress) + .build(); + + const result = Erc20BalanceSchema.safeParse(erc20Balance); + + expect(result.success && result.data.tokenAddress).toBe( + getAddress(nonChecksummedTokenAddress), + ); + }); + + it('should not allow an invalid ERC-20 balance', () => { + const erc20Balance = { invalid: 'erc20Balance' }; + + const result = NativeBalanceSchema.safeParse(erc20Balance); + + expect(!result.success && result.error).toStrictEqual( + new ZodError([ + { + code: 'invalid_type', + expected: 'string', + received: 'undefined', + path: ['balance'], + message: 'Required', + }, + ]), + ); + }); + }); + + describe('BalanceSchema', () => { + it('should validate a valid native balance', () => { + const nativeBalance = balanceBuilder() + .with('tokenAddress', null) + .with('token', null) + .build(); + + const result = BalanceSchema.safeParse(nativeBalance); + + expect(result.success).toBe(true); + }); + + it('should validate a valid ERC-20 balance', () => { + const erc20Balance = balanceBuilder().build(); + + const result = BalanceSchema.safeParse(erc20Balance); + + expect(result.success).toBe(true); + }); + + it('should not allow an invalid balance', () => { + const balance = { invalid: 'balance' }; + + const result = BalanceSchema.safeParse(balance); + + expect(!result.success && result.error).toStrictEqual( + new ZodError([ + { + code: 'invalid_union', + unionErrors: [ + // @ts-expect-error - inferred types don't allow optional fields + { + issues: [ + { + code: 'invalid_type', + expected: 'string', + received: 'undefined', + path: ['balance'], + message: 'Required', + }, + ], + name: 'ZodError', + }, + // @ts-expect-error - inferred types don't allow optional fields + { + issues: [ + { + code: 'invalid_type', + expected: 'string', + received: 'undefined', + path: ['tokenAddress'], + message: 'Required', + }, + { + code: 'invalid_type', + expected: 'object', + received: 'undefined', + path: ['token'], + message: 'Required', + }, + { + code: 'invalid_type', + expected: 'string', + received: 'undefined', + path: ['balance'], + message: 'Required', + }, + ], + name: 'ZodError', + }, + ], + path: [], + message: 'Invalid input', + }, + ]), + ); + }); + + it('should allow optional fiatBalance and fiatConversion and default to null', () => { + const nativeBalance = balanceBuilder().build(); + // @ts-expect-error - inferred types don't allow optional fields + delete nativeBalance.fiatBalance; + // @ts-expect-error - inferred types don't allow optional fields + delete nativeBalance.fiatConversion; + + const result = BalanceSchema.safeParse(nativeBalance); + + expect(result.success && result.data.fiatBalance).toBe(null); + expect(result.success && result.data.fiatConversion).toBe(null); + }); + + it('should now allow a malformed balance-type', () => { + const nativeBalance = balanceBuilder().build(); + // @ts-expect-error - inferred types don't allow optional fields + delete nativeBalance.token; // Native balance-like, but ERC-20 balance requires it + + const result = BalanceSchema.safeParse(nativeBalance); + + expect(!result.success && result.error).toStrictEqual( + new ZodError([ + { + code: 'invalid_union', + unionErrors: [ + // @ts-expect-error - inferred types don't allow optional fields + { + issues: [ + { + code: 'invalid_type', + expected: 'null', + received: 'string', + path: ['tokenAddress'], + message: 'Expected null, received string', + }, + ], + name: 'ZodError', + }, + // @ts-expect-error - inferred types don't allow optional fields + { + issues: [ + { + code: 'invalid_type', + expected: 'object', + received: 'undefined', + path: ['token'], + message: 'Required', + }, + ], + name: 'ZodError', + }, + ], + path: [], + message: 'Invalid input', + }, + ]), + ); + }); + }); +}); diff --git a/src/domain/balances/entities/schemas/balance.schema.ts b/src/domain/balances/entities/schemas/balance.schema.ts index 8589effc37..da26b763ad 100644 --- a/src/domain/balances/entities/schemas/balance.schema.ts +++ b/src/domain/balances/entities/schemas/balance.schema.ts @@ -2,8 +2,9 @@ import { z } from 'zod'; import { AddressSchema } from '@/validation/entities/schemas/address.schema'; export const NativeBalanceSchema = z.object({ - tokenAddress: z.null(), - token: z.null(), + // Likely `null` but for safety we allow optional defaulting + tokenAddress: z.null().optional().default(null), + token: z.null().optional().default(null), balance: z.string(), }); diff --git a/src/routes/balances/balances.controller.spec.ts b/src/routes/balances/balances.controller.spec.ts index 5424298a22..1c85e582c5 100644 --- a/src/routes/balances/balances.controller.spec.ts +++ b/src/routes/balances/balances.controller.spec.ts @@ -641,10 +641,10 @@ describe('Balances Controller (Unit)', () => { .expect({ statusCode: 422, code: 'invalid_type', - expected: 'null', - message: 'Required', - path: ['tokenAddress'], + expected: 'string', received: 'undefined', + path: ['balance'], + message: 'Required', }); expect(networkService.get.mock.calls.length).toBe(3); From ed7ad09cd0585265c5b2b55e2b1908c2ed0cd72b Mon Sep 17 00:00:00 2001 From: iamacook Date: Thu, 7 Mar 2024 16:24:15 +0100 Subject: [PATCH 5/6] Use `nullish` --- src/domain/balances/entities/schemas/balance.schema.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/domain/balances/entities/schemas/balance.schema.ts b/src/domain/balances/entities/schemas/balance.schema.ts index da26b763ad..04e0b831e4 100644 --- a/src/domain/balances/entities/schemas/balance.schema.ts +++ b/src/domain/balances/entities/schemas/balance.schema.ts @@ -22,8 +22,8 @@ export const Erc20BalanceSchema = z.object({ }); const FiatSchema = z.object({ - fiatBalance: z.string().optional().nullable().default(null), - fiatConversion: z.string().optional().nullable().default(null), + fiatBalance: z.string().nullish().default(null), + fiatConversion: z.string().nullish().default(null), }); export const BalanceSchema = z.union([ From 67f73902467a210bc0d561f447e1b5a4640e426e Mon Sep 17 00:00:00 2001 From: iamacook Date: Mon, 11 Mar 2024 10:17:21 +0100 Subject: [PATCH 6/6] Update comments --- .../entities/schemas/__tests__/balance.schema.spec.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/domain/balances/entities/schemas/__tests__/balance.schema.spec.ts b/src/domain/balances/entities/schemas/__tests__/balance.schema.spec.ts index 95660ef1dd..cce4f0b7fc 100644 --- a/src/domain/balances/entities/schemas/__tests__/balance.schema.spec.ts +++ b/src/domain/balances/entities/schemas/__tests__/balance.schema.spec.ts @@ -177,7 +177,7 @@ describe('Balances schema', () => { { code: 'invalid_union', unionErrors: [ - // @ts-expect-error - inferred types don't allow optional fields + // @ts-expect-error - unionError is missing some properties (zod-based error) { issues: [ { @@ -190,7 +190,7 @@ describe('Balances schema', () => { ], name: 'ZodError', }, - // @ts-expect-error - inferred types don't allow optional fields + // @ts-expect-error - unionError is missing some properties (zod-based error) { issues: [ { @@ -250,7 +250,7 @@ describe('Balances schema', () => { { code: 'invalid_union', unionErrors: [ - // @ts-expect-error - inferred types don't allow optional fields + // @ts-expect-error - unionError is missing some properties (zod-based error) { issues: [ { @@ -263,7 +263,7 @@ describe('Balances schema', () => { ], name: 'ZodError', }, - // @ts-expect-error - inferred types don't allow optional fields + // @ts-expect-error - unionError is missing some properties (zod-based error) { issues: [ {