From edc276d039c754c0bab0662bd2e6a6eddeffc8da Mon Sep 17 00:00:00 2001 From: Aaron Cook Date: Fri, 22 Mar 2024 15:13:27 +0100 Subject: [PATCH] Migrate `Message`/`MessageConfirmation` to `zod` (#1315) This migrates the validation of `Message` to `zod`: - Remove `MessageValidator` and associated schemas. - Add test-covered `MessageSchema`/`MessageConfirmationSchema` and infer types from them. - Propagate type requirements. - Update tests accordingly. --- src/domain.module.ts | 2 - .../__tests__/message-confirmation.builder.ts | 8 +- .../entities/__tests__/message.builder.ts | 15 +- .../entities/message-confirmation.entity.ts | 11 +- .../messages/entities/message.entity.ts | 15 +- .../schemas/__tests__/message.schema.spec.ts | 220 ++++++++++++++++++ .../entities/schemas/message.schema.ts | 78 ++----- src/domain/messages/message.validator.ts | 50 ---- src/domain/messages/messages.repository.ts | 10 +- .../entities/message-confirmation.entity.ts | 4 +- .../messages/entities/message.entity.ts | 12 +- .../messages/messages.controller.spec.ts | 5 +- 12 files changed, 284 insertions(+), 146 deletions(-) create mode 100644 src/domain/messages/entities/schemas/__tests__/message.schema.spec.ts delete mode 100644 src/domain/messages/message.validator.ts diff --git a/src/domain.module.ts b/src/domain.module.ts index 20d80d022d..e14193d6c2 100644 --- a/src/domain.module.ts +++ b/src/domain.module.ts @@ -31,7 +31,6 @@ import { IEstimationsRepository } from '@/domain/estimations/estimations.reposit import { EstimationsRepository } from '@/domain/estimations/estimations.repository'; import { MessagesRepository } from '@/domain/messages/messages.repository'; import { IMessagesRepository } from '@/domain/messages/messages.repository.interface'; -import { MessageValidator } from '@/domain/messages/message.validator'; import { IHealthRepository } from '@/domain/health/health.repository.interface'; import { HealthRepository } from '@/domain/health/health.repository'; import { HumanDescriptionApiModule } from '@/datasources/human-description-api/human-description-api.module'; @@ -67,7 +66,6 @@ import { BalancesApiModule } from '@/datasources/balances-api/balances-api.modul { provide: ISafeRepository, useClass: SafeRepository }, { provide: ITokenRepository, useClass: TokenRepository }, DataDecodedValidator, - MessageValidator, MultisigTransactionValidator, TransactionTypeValidator, TransferValidator, diff --git a/src/domain/messages/entities/__tests__/message-confirmation.builder.ts b/src/domain/messages/entities/__tests__/message-confirmation.builder.ts index 628eba4a94..785ab170cc 100644 --- a/src/domain/messages/entities/__tests__/message-confirmation.builder.ts +++ b/src/domain/messages/entities/__tests__/message-confirmation.builder.ts @@ -4,13 +4,17 @@ import { MessageConfirmation, SignatureType, } from '@/domain/messages/entities/message-confirmation.entity'; +import { getAddress } from 'viem'; export function messageConfirmationBuilder(): IBuilder { return new Builder() .with('created', faker.date.recent()) .with('modified', faker.date.recent()) - .with('owner', faker.finance.ethereumAddress()) - .with('signature', faker.string.hexadecimal({ length: 32 })) + .with('owner', getAddress(faker.finance.ethereumAddress())) + .with( + 'signature', + faker.string.hexadecimal({ length: 32 }) as `0x${string}`, + ) .with('signatureType', faker.helpers.objectValue(SignatureType)); } diff --git a/src/domain/messages/entities/__tests__/message.builder.ts b/src/domain/messages/entities/__tests__/message.builder.ts index 069f8a5b15..52dc637365 100644 --- a/src/domain/messages/entities/__tests__/message.builder.ts +++ b/src/domain/messages/entities/__tests__/message.builder.ts @@ -5,15 +5,19 @@ import { messageConfirmationBuilder, toJson as messageConfirmationToJson, } from '@/domain/messages/entities/__tests__/message-confirmation.builder'; +import { getAddress } from 'viem'; export function messageBuilder(): IBuilder { return new Builder() .with('created', faker.date.recent()) .with('modified', faker.date.recent()) - .with('safe', faker.finance.ethereumAddress()) + .with('safe', getAddress(faker.finance.ethereumAddress())) .with('message', faker.word.words({ count: { min: 1, max: 5 } })) - .with('messageHash', faker.string.hexadecimal({ length: 32 })) - .with('proposedBy', faker.finance.ethereumAddress()) + .with( + 'messageHash', + faker.string.hexadecimal({ length: 32 }) as `0x${string}`, + ) + .with('proposedBy', getAddress(faker.finance.ethereumAddress())) .with('safeAppId', faker.number.int()) .with( 'confirmations', @@ -21,7 +25,10 @@ export function messageBuilder(): IBuilder { count: { min: 2, max: 5 }, }), ) - .with('preparedSignature', faker.string.hexadecimal({ length: 32 })); + .with( + 'preparedSignature', + faker.string.hexadecimal({ length: 32 }) as `0x${string}`, + ); } export function toJson(message: Message): unknown { diff --git a/src/domain/messages/entities/message-confirmation.entity.ts b/src/domain/messages/entities/message-confirmation.entity.ts index 07b46c24c8..7bef80f10d 100644 --- a/src/domain/messages/entities/message-confirmation.entity.ts +++ b/src/domain/messages/entities/message-confirmation.entity.ts @@ -1,3 +1,6 @@ +import { MessageConfirmationSchema } from '@/domain/messages/entities/schemas/message.schema'; +import { z } from 'zod'; + export enum SignatureType { ContractSignature = 'CONTRACT_SIGNATURE', ApprovedHash = 'APPROVED_HASH', @@ -5,10 +8,4 @@ export enum SignatureType { EthSign = 'ETH_SIGN', } -export interface MessageConfirmation { - created: Date; - modified: Date; - owner: string; - signature: string; - signatureType: SignatureType; -} +export type MessageConfirmation = z.infer; diff --git a/src/domain/messages/entities/message.entity.ts b/src/domain/messages/entities/message.entity.ts index 49093f4620..ff86227fba 100644 --- a/src/domain/messages/entities/message.entity.ts +++ b/src/domain/messages/entities/message.entity.ts @@ -1,13 +1,4 @@ -import { MessageConfirmation } from '@/domain/messages/entities/message-confirmation.entity'; +import { MessageSchema } from '@/domain/messages/entities/schemas/message.schema'; +import { z } from 'zod'; -export interface Message { - created: Date; - modified: Date; - safe: string; - messageHash: string; - message: string | unknown; - proposedBy: string; - safeAppId: number | null; - confirmations: MessageConfirmation[]; - preparedSignature: string | null; -} +export type Message = z.infer; diff --git a/src/domain/messages/entities/schemas/__tests__/message.schema.spec.ts b/src/domain/messages/entities/schemas/__tests__/message.schema.spec.ts new file mode 100644 index 0000000000..ae14f84ed3 --- /dev/null +++ b/src/domain/messages/entities/schemas/__tests__/message.schema.spec.ts @@ -0,0 +1,220 @@ +import { fakeJson } from '@/__tests__/faker'; +import { pageBuilder } from '@/domain/entities/__tests__/page.builder'; +import { messageConfirmationBuilder } from '@/domain/messages/entities/__tests__/message-confirmation.builder'; +import { messageBuilder } from '@/domain/messages/entities/__tests__/message.builder'; +import { SignatureType } from '@/domain/messages/entities/message-confirmation.entity'; +import { + MessageConfirmationSchema, + MessagePageSchema, + MessageSchema, +} from '@/domain/messages/entities/schemas/message.schema'; +import { faker } from '@faker-js/faker'; +import { getAddress } from 'viem'; +import { ZodError } from 'zod'; + +describe('Message schemas', () => { + describe('MessageConfirmationSchema', () => { + it('should validate a valid MessageConfirmation', () => { + const messageConfirmation = messageConfirmationBuilder().build(); + + const result = MessageConfirmationSchema.safeParse(messageConfirmation); + + expect(result.success).toBe(true); + }); + + it.each(['created' as const, 'modified' as const])( + 'should coerce %s to a date', + (key) => { + const messageConfirmation = messageConfirmationBuilder() + .with(key, faker.date.recent().toISOString() as unknown as Date) + .build(); + + const result = MessageConfirmationSchema.safeParse(messageConfirmation); + + expect(result.success && result.data[key]).toStrictEqual( + new Date(messageConfirmation[key]), + ); + }, + ); + + it('should checksum the owner', () => { + const nonChecksummedAddress = faker.finance + .ethereumAddress() + .toLowerCase() as `0x${string}`; + const messageConfirmation = messageConfirmationBuilder() + .with('owner', nonChecksummedAddress) + .build(); + + const result = MessageConfirmationSchema.safeParse(messageConfirmation); + + expect(result.success && result.data.owner).toBe( + getAddress(nonChecksummedAddress), + ); + }); + + it('should not allow non-hex signature', () => { + const messageConfirmation = messageConfirmationBuilder() + .with('signature', faker.string.numeric() as `0x${string}`) + .build(); + + const result = MessageConfirmationSchema.safeParse(messageConfirmation); + + expect(!result.success && result.error).toStrictEqual( + new ZodError([ + { + code: 'custom', + message: 'Invalid input', + path: ['signature'], + }, + ]), + ); + }); + + it('should not allow invalid signature types', () => { + const messageConfirmation = messageConfirmationBuilder() + .with('signatureType', faker.lorem.word() as SignatureType) + .build(); + + const result = MessageConfirmationSchema.safeParse(messageConfirmation); + + expect(!result.success && result.error).toStrictEqual( + new ZodError([ + { + received: messageConfirmation.signatureType, + code: 'invalid_enum_value', + options: ['CONTRACT_SIGNATURE', 'APPROVED_HASH', 'EOA', 'ETH_SIGN'], + path: ['signatureType'], + message: `Invalid enum value. Expected 'CONTRACT_SIGNATURE' | 'APPROVED_HASH' | 'EOA' | 'ETH_SIGN', received '${messageConfirmation.signatureType}'`, + }, + ]), + ); + }); + }); + + describe('MessageSchema', () => { + it('should validate a valid Message', () => { + const message = messageBuilder().build(); + + const result = MessageSchema.safeParse(message); + + expect(result.success).toBe(true); + }); + + it.each(['created' as const, 'modified' as const])( + 'should coerce %s to a date', + (key) => { + const message = messageBuilder().build(); + + const result = MessageSchema.safeParse(message); + + expect(result.success && result.data[key]).toStrictEqual( + new Date(message[key]), + ); + }, + ); + + it.each(['safe' as const, 'proposedBy' as const])( + 'should checksum the %s', + (key) => { + const nonChecksummedAddress = faker.finance + .ethereumAddress() + .toLowerCase() as `0x${string}`; + const message = messageBuilder() + .with(key, nonChecksummedAddress) + .build(); + + const result = MessageSchema.safeParse(message); + + expect(result.success && result.data[key]).toBe( + getAddress(nonChecksummedAddress), + ); + }, + ); + + it.each(['messageHash' as const, 'preparedSignature' as const])( + 'should not allow non-hex %s', + (key) => { + const message = messageBuilder() + .with(key, faker.string.numeric() as `0x${string}`) + .build(); + + const result = MessageSchema.safeParse(message); + + expect(!result.success && result.error).toStrictEqual( + new ZodError([ + { + code: 'custom', + message: 'Invalid input', + path: [key], + }, + ]), + ); + }, + ); + + it.each([ + ['string', faker.lorem.sentence()], + ['object', JSON.parse(fakeJson())], + ])('should allow a %s message', (_, message) => { + const result = MessageSchema.safeParse({ + ...messageBuilder().build(), + message, + }); + + expect(result.success).toBe(true); + }); + + it.each(['safeAppId' as const, 'preparedSignature' as const])( + 'should allow undefined %s, defaulting to null', + (key) => { + const message = messageBuilder().build(); + delete message[key]; + + const result = MessageSchema.safeParse(message); + + expect(result.success && result.data[key]).toBe(null); + }, + ); + + it('should allow empty confirmations', () => { + const message = messageBuilder().with('confirmations', []).build(); + + const result = MessageSchema.safeParse(message); + + expect(result.success).toBe(true); + }); + + it.each([ + 'created' as const, + 'modified' as const, + 'safe' as const, + 'messageHash' as const, + 'message' as const, + 'proposedBy' as const, + 'confirmations' as const, + ])('should not allow %s to be undefined', (key) => { + const message = messageBuilder().build(); + delete message[key]; + + const result = MessageSchema.safeParse(message); + + expect( + !result.success && + result.error.issues.length === 1 && + result.error.issues[0].path.length === 1 && + result.error.issues[0].path[0] === key, + ).toBe(true); + }); + }); + + describe('MessagePageSchema', () => { + it('should validate a valid Page', () => { + const message = messageBuilder().build(); + const messagePage = pageBuilder().with('results', [message]).build(); + + const result = MessagePageSchema.safeParse(messagePage); + + expect(result.success).toBe(true); + }); + }); +}); diff --git a/src/domain/messages/entities/schemas/message.schema.ts b/src/domain/messages/entities/schemas/message.schema.ts index d171ce6a1e..043bb4c9b1 100644 --- a/src/domain/messages/entities/schemas/message.schema.ts +++ b/src/domain/messages/entities/schemas/message.schema.ts @@ -1,57 +1,27 @@ -import { buildPageSchema } from '@/domain/entities/schemas/page.schema.factory'; -import { Schema } from 'ajv'; +import { buildZodPageSchema } from '@/domain/entities/schemas/page.schema.factory'; +import { SignatureType } from '@/domain/messages/entities/message-confirmation.entity'; +import { AddressSchema } from '@/validation/entities/schemas/address.schema'; +import { HexSchema } from '@/validation/entities/schemas/hex.schema'; +import { z } from 'zod'; -export const MESSAGE_CONFIRMATION_SCHEMA_ID = - 'https://safe-client.safe.global/schemas/messages/message-confirmation.json'; +export const MessageConfirmationSchema = z.object({ + created: z.coerce.date(), + modified: z.coerce.date(), + owner: AddressSchema, + signature: HexSchema, + signatureType: z.nativeEnum(SignatureType), +}); -export const messageConfirmationSchema: Schema = { - $id: MESSAGE_CONFIRMATION_SCHEMA_ID, - type: 'object', - properties: { - created: { type: 'string', isDate: true }, - modified: { type: 'string', isDate: true }, - owner: { type: 'string' }, - signature: { type: 'string' }, - signatureType: { type: 'string' }, - }, - required: ['created', 'modified', 'owner', 'signature', 'signatureType'], -}; +export const MessageSchema = z.object({ + created: z.coerce.date(), + modified: z.coerce.date(), + safe: AddressSchema, + messageHash: HexSchema, + message: z.union([z.string(), z.record(z.unknown())]), + proposedBy: AddressSchema, + safeAppId: z.number().nullish().default(null), + confirmations: z.array(MessageConfirmationSchema), + preparedSignature: HexSchema.nullish().default(null), +}); -export const MESSAGE_SCHEMA_ID = - 'https://safe-client.safe.global/schemas/messages/message.json'; - -export const messageSchema: Schema = { - $id: MESSAGE_SCHEMA_ID, - type: 'object', - properties: { - created: { type: 'string', isDate: true }, - modified: { type: 'string', isDate: true }, - safe: { type: 'string' }, - messageHash: { type: 'string' }, - message: { type: ['object', 'string'] }, - proposedBy: { type: 'string' }, - safeAppId: { type: 'number', nullable: true }, - confirmations: { - type: 'array', - items: { $ref: 'message-confirmation.json' }, - }, - preparedSignature: { type: 'string', nullable: true }, - }, - required: [ - 'created', - 'modified', - 'safe', - 'messageHash', - 'message', - 'proposedBy', - 'confirmations', - ], -}; - -export const MESSAGE_PAGE_SCHEMA_ID = - 'https://safe-client.safe.global/schemas/messages/message-page.json'; - -export const messagePageSchema: Schema = buildPageSchema( - MESSAGE_PAGE_SCHEMA_ID, - messageSchema, -); +export const MessagePageSchema = buildZodPageSchema(MessageSchema); diff --git a/src/domain/messages/message.validator.ts b/src/domain/messages/message.validator.ts deleted file mode 100644 index 553e42d651..0000000000 --- a/src/domain/messages/message.validator.ts +++ /dev/null @@ -1,50 +0,0 @@ -import { Injectable } from '@nestjs/common'; -import { ValidateFunction } from 'ajv'; -import { IPageValidator } from '@/domain/interfaces/page-validator.interface'; -import { IValidator } from '@/domain/interfaces/validator.interface'; -import { Message } from '@/domain/messages/entities/message.entity'; -import { - MESSAGE_CONFIRMATION_SCHEMA_ID, - MESSAGE_PAGE_SCHEMA_ID, - MESSAGE_SCHEMA_ID, - messageConfirmationSchema, - messagePageSchema, - messageSchema, -} from '@/domain/messages/entities/schemas/message.schema'; -import { GenericValidator } from '@/validation/providers/generic.validator'; -import { JsonSchemaService } from '@/validation/providers/json-schema.service'; -import { Page } from '@/domain/entities/page.entity'; - -@Injectable() -export class MessageValidator - implements IValidator, IPageValidator -{ - private readonly isValidMessage: ValidateFunction; - private readonly isValidPage: ValidateFunction>; - - constructor( - private readonly genericValidator: GenericValidator, - private readonly jsonSchemaValidator: JsonSchemaService, - ) { - this.jsonSchemaValidator.getSchema( - MESSAGE_CONFIRMATION_SCHEMA_ID, - messageConfirmationSchema, - ); - this.isValidMessage = this.jsonSchemaValidator.getSchema( - MESSAGE_SCHEMA_ID, - messageSchema, - ); - this.isValidPage = this.jsonSchemaValidator.getSchema( - MESSAGE_PAGE_SCHEMA_ID, - messagePageSchema, - ); - } - - validate(data: unknown): Message { - return this.genericValidator.validate(this.isValidMessage, data); - } - - validatePage(data: unknown): Page { - return this.genericValidator.validate(this.isValidPage, data); - } -} diff --git a/src/domain/messages/messages.repository.ts b/src/domain/messages/messages.repository.ts index e7b23bf964..b1d8e68b90 100644 --- a/src/domain/messages/messages.repository.ts +++ b/src/domain/messages/messages.repository.ts @@ -2,15 +2,17 @@ import { Inject, Injectable } from '@nestjs/common'; import { Page } from '@/domain/entities/page.entity'; import { ITransactionApiManager } from '@/domain/interfaces/transaction-api.manager.interface'; import { Message } from '@/domain/messages/entities/message.entity'; -import { MessageValidator } from '@/domain/messages/message.validator'; import { IMessagesRepository } from '@/domain/messages/messages.repository.interface'; +import { + MessagePageSchema, + MessageSchema, +} from '@/domain/messages/entities/schemas/message.schema'; @Injectable() export class MessagesRepository implements IMessagesRepository { constructor( @Inject(ITransactionApiManager) private readonly transactionApiManager: ITransactionApiManager, - private readonly messageValidator: MessageValidator, ) {} async getMessageByHash(args: { @@ -20,7 +22,7 @@ export class MessagesRepository implements IMessagesRepository { const transactionService = await this.transactionApiManager.getTransactionApi(args.chainId); const message = await transactionService.getMessageByHash(args.messageHash); - return this.messageValidator.validate(message); + return MessageSchema.parse(message); } async getMessagesBySafe(args: { @@ -37,7 +39,7 @@ export class MessagesRepository implements IMessagesRepository { offset: args.offset, }); - return this.messageValidator.validatePage(page); + return MessagePageSchema.parse(page); } async createMessage(args: { diff --git a/src/routes/messages/entities/message-confirmation.entity.ts b/src/routes/messages/entities/message-confirmation.entity.ts index 06efba8d7a..65316e2687 100644 --- a/src/routes/messages/entities/message-confirmation.entity.ts +++ b/src/routes/messages/entities/message-confirmation.entity.ts @@ -5,9 +5,9 @@ export class MessageConfirmation { @ApiProperty() owner: AddressInfo; @ApiProperty() - signature: string; + signature: `0x${string}`; - constructor(owner: AddressInfo, signature: string) { + constructor(owner: AddressInfo, signature: `0x${string}`) { this.owner = owner; this.signature = signature; } diff --git a/src/routes/messages/entities/message.entity.ts b/src/routes/messages/entities/message.entity.ts index d7fbf2c45f..5e62bc51a0 100644 --- a/src/routes/messages/entities/message.entity.ts +++ b/src/routes/messages/entities/message.entity.ts @@ -9,7 +9,7 @@ export enum MessageStatus { export class Message { @ApiProperty() - messageHash: string; + messageHash: `0x${string}`; @ApiProperty() status: MessageStatus; @ApiPropertyOptional({ type: String, nullable: true }) @@ -17,7 +17,7 @@ export class Message { @ApiPropertyOptional({ type: String, nullable: true }) name: string | null; @ApiProperty() - message: string | unknown; + message: string | Record; @ApiProperty() creationTimestamp: number; @ApiProperty() @@ -31,21 +31,21 @@ export class Message { @ApiProperty() confirmations: MessageConfirmation[]; @ApiPropertyOptional({ type: String, nullable: true }) - preparedSignature: string | null; + preparedSignature: `0x${string}` | null; constructor( - messageHash: string, + messageHash: `0x${string}`, status: MessageStatus, logoUri: string | null, name: string | null, - message: string | unknown, + message: string | Record, creationTimestamp: number, modifiedTimestamp: number, confirmationsSubmitted: number, confirmationsRequired: number, proposedBy: AddressInfo, confirmations: MessageConfirmation[], - preparedSignature: string | null, + preparedSignature: `0x${string}` | null, ) { this.messageHash = messageHash; this.status = status; diff --git a/src/routes/messages/messages.controller.spec.ts b/src/routes/messages/messages.controller.spec.ts index 555112e0cb..f854139741 100644 --- a/src/routes/messages/messages.controller.spec.ts +++ b/src/routes/messages/messages.controller.spec.ts @@ -476,7 +476,7 @@ describe('Messages controller', () => { await request(app.getHttpServer()) .get(`/v1/chains/${chain.chainId}/safes/${safe.address}/messages`) .expect(500) - .expect({ message: 'Validation failed', code: 42, arguments: [] }); + .expect({ statusCode: 500, message: 'Internal server error' }); }); it('should get a message with a date label', async () => { @@ -804,11 +804,10 @@ describe('Messages controller', () => { const chain = chainBuilder().build(); const safe = safeBuilder().build(); const createMessageDto = messageBuilder().build(); - createMessageDto.message = faker.number.int(); await request(app.getHttpServer()) .post(`/v1/chains/${chain.chainId}/safes/${safe.address}/messages`) - .send(createMessageDto) + .send({ ...createMessageDto, message: faker.number.int() }) .expect(422) .expect({ statusCode: 422,