From b8539322780b888222d7944c6315c37ffc71adfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hector=20G=C3=B3mez=20Varela?= Date: Tue, 30 Jul 2024 17:52:12 +0200 Subject: [PATCH] PR comments --- ...unterfactual-safes.repository.interface.ts | 2 +- .../counterfactual-safes.repository.ts | 47 ++++++++-------- .../__tests__/account-data-type.builder.ts | 14 ++++- .../entities/account-data-type.entity.spec.ts | 54 ++++++++++++------- .../entities/account-data-type.entity.ts | 8 ++- .../counterfactual-safes.controller.spec.ts | 28 ++++++---- .../counterfactual-safes.controller.ts | 4 +- .../counterfactual-safes.service.ts | 5 +- .../entities/counterfactual-safe.entity.ts | 4 -- 9 files changed, 99 insertions(+), 67 deletions(-) diff --git a/src/domain/accounts/counterfactual-safes/counterfactual-safes.repository.interface.ts b/src/domain/accounts/counterfactual-safes/counterfactual-safes.repository.interface.ts index 0c3cacd850..26529f35d7 100644 --- a/src/domain/accounts/counterfactual-safes/counterfactual-safes.repository.interface.ts +++ b/src/domain/accounts/counterfactual-safes/counterfactual-safes.repository.interface.ts @@ -18,7 +18,7 @@ export interface ICounterfactualSafesRepository { predictedAddress: `0x${string}`; }): Promise; - getOrCreateCounterfactualSafe(args: { + upsertCounterfactualSafe(args: { authPayload: AuthPayload; address: `0x${string}`; createCounterfactualSafeDto: CreateCounterfactualSafeDto; diff --git a/src/domain/accounts/counterfactual-safes/counterfactual-safes.repository.ts b/src/domain/accounts/counterfactual-safes/counterfactual-safes.repository.ts index d803c32495..4743ee5345 100644 --- a/src/domain/accounts/counterfactual-safes/counterfactual-safes.repository.ts +++ b/src/domain/accounts/counterfactual-safes/counterfactual-safes.repository.ts @@ -2,7 +2,10 @@ import { IAccountsRepository } from '@/domain/accounts/accounts.repository.inter import { ICounterfactualSafesRepository } from '@/domain/accounts/counterfactual-safes/counterfactual-safes.repository.interface'; import { CounterfactualSafe } from '@/domain/accounts/counterfactual-safes/entities/counterfactual-safe.entity'; import { CreateCounterfactualSafeDto } from '@/domain/accounts/counterfactual-safes/entities/create-counterfactual-safe.dto.entity'; -import { AccountDataType } from '@/domain/accounts/entities/account-data-type.entity'; +import { + AccountDataType, + AccountDataTypeNames, +} from '@/domain/accounts/entities/account-data-type.entity'; import { AuthPayload } from '@/domain/auth/entities/auth-payload.entity'; import { ICounterfactualSafesDatasource } from '@/domain/interfaces/counterfactual-safes.datasource.interface'; import { LoggingService, ILoggingService } from '@/logging/logging.interface'; @@ -39,16 +42,14 @@ export class CounterfactualSafesRepository if (!args.authPayload.isForSigner(args.address)) { throw new UnauthorizedException(); } - const [account] = await Promise.all([ - this.accountsRepository.getAccount({ - authPayload: args.authPayload, - address: args.address, - }), - this.checkCounterfactualSafesIsEnabled({ - authPayload: args.authPayload, - address: args.address, - }), - ]); + await this.checkCounterfactualSafesIsEnabled({ + authPayload: args.authPayload, + address: args.address, + }); + const account = await this.accountsRepository.getAccount({ + authPayload: args.authPayload, + address: args.address, + }); return this.datasource.getCounterfactualSafe({ account, chainId: args.chainId, @@ -63,7 +64,7 @@ export class CounterfactualSafesRepository * If the Counterfactual Safe exists, it returns it. * If the Counterfactual Safe does not exist, it's created. */ - async getOrCreateCounterfactualSafe(args: { + async upsertCounterfactualSafe(args: { authPayload: AuthPayload; address: `0x${string}`; createCounterfactualSafeDto: CreateCounterfactualSafeDto; @@ -71,16 +72,14 @@ export class CounterfactualSafesRepository if (!args.authPayload.isForSigner(args.address)) { throw new UnauthorizedException(); } - const [account] = await Promise.all([ - this.accountsRepository.getAccount({ - authPayload: args.authPayload, - address: args.address, - }), - this.checkCounterfactualSafesIsEnabled({ - authPayload: args.authPayload, - address: args.address, - }), - ]); + await this.checkCounterfactualSafesIsEnabled({ + authPayload: args.authPayload, + address: args.address, + }); + const account = await this.accountsRepository.getAccount({ + authPayload: args.authPayload, + address: args.address, + }); try { return await this.datasource.getCounterfactualSafe({ @@ -125,11 +124,11 @@ export class CounterfactualSafesRepository private async checkCounterfactualSafeDataTypeIsActive(): Promise { const dataTypes = await this.accountsRepository.getDataTypes(); const counterfactualSafeDataType = dataTypes.find( - (dataType) => dataType.name === 'CounterfactualSafes', + (dataType) => dataType.name === AccountDataTypeNames.CounterfactualSafes, ); if (!counterfactualSafeDataType?.is_active) { this.loggingService.warn({ - message: 'CounterfactualSafes data type is not active', + message: `${AccountDataTypeNames.CounterfactualSafes} data type is not active`, }); throw new GoneException(); } diff --git a/src/domain/accounts/entities/__tests__/account-data-type.builder.ts b/src/domain/accounts/entities/__tests__/account-data-type.builder.ts index 49bc84d712..084a927281 100644 --- a/src/domain/accounts/entities/__tests__/account-data-type.builder.ts +++ b/src/domain/accounts/entities/__tests__/account-data-type.builder.ts @@ -1,11 +1,21 @@ import { Builder, IBuilder } from '@/__tests__/builder'; -import { AccountDataType } from '@/domain/accounts/entities/account-data-type.entity'; +import { + AccountDataType, + AccountDataTypeNames, +} from '@/domain/accounts/entities/account-data-type.entity'; import { faker } from '@faker-js/faker'; export function accountDataTypeBuilder(): IBuilder { return new Builder() .with('id', faker.number.int()) - .with('name', faker.lorem.slug()) + .with( + 'name', + faker.helpers.arrayElement([ + AccountDataTypeNames.CounterfactualSafes, + AccountDataTypeNames.AddressBook, + AccountDataTypeNames.Watchlist, + ]), + ) .with('description', faker.lorem.slug()) .with('is_active', faker.datatype.boolean()) .with('created_at', faker.date.recent()) diff --git a/src/domain/accounts/entities/account-data-type.entity.spec.ts b/src/domain/accounts/entities/account-data-type.entity.spec.ts index 746d608bb4..bb30a21c33 100644 --- a/src/domain/accounts/entities/account-data-type.entity.spec.ts +++ b/src/domain/accounts/entities/account-data-type.entity.spec.ts @@ -32,26 +32,42 @@ describe('AccountDataTypeSchema', () => { }, ); - it.each(['name' as const, 'description' as const])( - 'should not verify an AccountDataType with a integer %s', - (field) => { - const accountDataType = accountDataTypeBuilder().build(); - // @ts-expect-error - should be strings - accountDataType[field] = faker.number.int(); + it('should not verify an AccountDataType with a integer description', () => { + const accountDataType = accountDataTypeBuilder().build(); + // @ts-expect-error - should be strings + accountDataType['description'] = faker.number.int(); - const result = AccountDataTypeSchema.safeParse(accountDataType); + const result = AccountDataTypeSchema.safeParse(accountDataType); - expect(!result.success && result.error.issues).toStrictEqual([ - { - code: 'invalid_type', - expected: 'string', - message: 'Expected string, received number', - path: [field], - received: 'number', - }, - ]); - }, - ); + expect(!result.success && result.error.issues).toStrictEqual([ + { + code: 'invalid_type', + expected: 'string', + message: 'Expected string, received number', + path: ['description'], + received: 'number', + }, + ]); + }); + + it('should not verify an AccountDataType with a random name', () => { + const accountDataType = accountDataTypeBuilder().build(); + const randomName = faker.string.sample(); + // @ts-expect-error - should be strings + accountDataType['name'] = randomName; + + const result = AccountDataTypeSchema.safeParse(accountDataType); + + expect(!result.success && result.error.issues).toStrictEqual([ + { + code: 'invalid_enum_value', + message: `Invalid enum value. Expected 'CounterfactualSafes' | 'AddressBook' | 'Watchlist', received '${randomName}'`, + options: ['CounterfactualSafes', 'AddressBook', 'Watchlist'], + path: ['name'], + received: randomName, + }, + ]); + }); it('should not verify an AccountDataType with a non-boolean is_active', () => { const accountDataType = accountDataTypeBuilder().build(); @@ -98,7 +114,7 @@ describe('AccountDataTypeSchema', () => { }, { code: 'invalid_type', - expected: 'string', + expected: "'CounterfactualSafes' | 'AddressBook' | 'Watchlist'", received: 'undefined', path: ['name'], message: 'Required', diff --git a/src/domain/accounts/entities/account-data-type.entity.ts b/src/domain/accounts/entities/account-data-type.entity.ts index d3b364ec5e..060dce4a69 100644 --- a/src/domain/accounts/entities/account-data-type.entity.ts +++ b/src/domain/accounts/entities/account-data-type.entity.ts @@ -1,10 +1,16 @@ import { RowSchema } from '@/datasources/db/entities/row.entity'; import { z } from 'zod'; +export enum AccountDataTypeNames { + CounterfactualSafes = 'CounterfactualSafes', + AddressBook = 'AddressBook', + Watchlist = 'Watchlist', +} + export type AccountDataType = z.infer; export const AccountDataTypeSchema = RowSchema.extend({ - name: z.string(), + name: z.nativeEnum(AccountDataTypeNames), description: z.string().nullish().default(null), is_active: z.boolean().default(true), }); diff --git a/src/routes/accounts/counterfactual-safes/counterfactual-safes.controller.spec.ts b/src/routes/accounts/counterfactual-safes/counterfactual-safes.controller.spec.ts index 2e3ffea379..402495b71a 100644 --- a/src/routes/accounts/counterfactual-safes/counterfactual-safes.controller.spec.ts +++ b/src/routes/accounts/counterfactual-safes/counterfactual-safes.controller.spec.ts @@ -23,6 +23,7 @@ import { createCounterfactualSafeDtoBuilder } from '@/domain/accounts/counterfac import { accountDataSettingBuilder } from '@/domain/accounts/entities/__tests__/account-data-setting.builder'; import { accountDataTypeBuilder } from '@/domain/accounts/entities/__tests__/account-data-type.builder'; import { accountBuilder } from '@/domain/accounts/entities/__tests__/account.builder'; +import { AccountDataTypeNames } from '@/domain/accounts/entities/account-data-type.entity'; import { authPayloadDtoBuilder } from '@/domain/auth/entities/__tests__/auth-payload-dto.entity.builder'; import { chainBuilder } from '@/domain/chains/entities/__tests__/chain.builder'; import { IAccountsDatasource } from '@/domain/interfaces/accounts.datasource.interface'; @@ -101,7 +102,7 @@ describe('CounterfactualSafesController', () => { const account = accountBuilder().build(); const accountDataTypes = [ accountDataTypeBuilder() - .with('name', 'CounterfactualSafes') + .with('name', AccountDataTypeNames.CounterfactualSafes) .with('is_active', true) .build(), ]; @@ -134,7 +135,6 @@ describe('CounterfactualSafesController', () => { saltNonce: counterfactualSafe.salt_nonce, singletonAddress: counterfactualSafe.singleton_address, threshold: counterfactualSafe.threshold, - accountId: counterfactualSafe.account_id.toString(), }); }); @@ -159,14 +159,16 @@ describe('CounterfactualSafesController', () => { const address = getAddress(faker.finance.ethereumAddress()); const chain = chainBuilder().build(); const counterfactualSafe = counterfactualSafeBuilder().build(); + const accessToken = 'invalid'; await request(app.getHttpServer()) .get( `/v1/accounts/${address}/storage/counterfactual-safes/${chain.chainId}/${counterfactualSafe.predicted_address}`, ) - .set('Cookie', ['access_token=invalid']) + .set('Cookie', [`access_token=${accessToken}`]) .expect(403); + expect(() => jwtService.verify(accessToken)).toThrow('jwt malformed'); expect(accountsRepository.getAccount).not.toHaveBeenCalled(); expect( counterfactualSafesDataSource.getCounterfactualSafe, @@ -193,6 +195,7 @@ describe('CounterfactualSafesController', () => { .set('Cookie', [`access_token=${accessToken}`]) .expect(403); + expect(() => jwtService.verify(accessToken)).toThrow('jwt not active'); expect(accountsRepository.getAccount).not.toHaveBeenCalled(); expect( counterfactualSafesDataSource.getCounterfactualSafe, @@ -212,6 +215,7 @@ describe('CounterfactualSafesController', () => { exp: faker.date.past(), }); + expect(() => jwtService.verify(accessToken)).toThrow('jwt expired'); await request(app.getHttpServer()) .get( `/v1/accounts/${address}/storage/counterfactual-safes/${chain.chainId}/${counterfactualSafe.predicted_address}`, @@ -226,7 +230,7 @@ describe('CounterfactualSafesController', () => { }); it('returns 403 if signer_address is not a valid Ethereum address', async () => { - const address = faker.string.hexadecimal() as `0x${string}`; + const address = faker.string.sample() as `0x${string}`; const chain = chainBuilder().build(); const counterfactualSafe = counterfactualSafeBuilder().build(); const authPayloadDto = authPayloadDtoBuilder() @@ -282,7 +286,7 @@ describe('CounterfactualSafesController', () => { const account = accountBuilder().build(); const accountDataTypes = [ accountDataTypeBuilder() - .with('name', 'CounterfactualSafes') + .with('name', AccountDataTypeNames.CounterfactualSafes) .with('is_active', true) .build(), ]; @@ -321,7 +325,7 @@ describe('CounterfactualSafesController', () => { const account = accountBuilder().build(); const accountDataTypes = [ accountDataTypeBuilder() - .with('name', 'CounterfactualSafes') + .with('name', AccountDataTypeNames.CounterfactualSafes) .with('is_active', true) .build(), ]; @@ -355,7 +359,6 @@ describe('CounterfactualSafesController', () => { saltNonce: counterfactualSafe.salt_nonce, singletonAddress: counterfactualSafe.singleton_address, threshold: counterfactualSafe.threshold, - accountId: counterfactualSafe.account_id.toString(), }); expect( @@ -381,7 +384,7 @@ describe('CounterfactualSafesController', () => { const account = accountBuilder().build(); const accountDataTypes = [ accountDataTypeBuilder() - .with('name', 'CounterfactualSafes') + .with('name', AccountDataTypeNames.CounterfactualSafes) .with('is_active', true) .build(), ]; @@ -418,7 +421,6 @@ describe('CounterfactualSafesController', () => { saltNonce: counterfactualSafe.salt_nonce, singletonAddress: counterfactualSafe.singleton_address, threshold: counterfactualSafe.threshold, - accountId: counterfactualSafe.account_id.toString(), }); expect( @@ -452,10 +454,12 @@ describe('CounterfactualSafesController', () => { const address = getAddress(faker.finance.ethereumAddress()); const createCounterfactualSafeDto = createCounterfactualSafeDtoBuilder().build(); + const accessToken = 'invalid'; + expect(() => jwtService.verify(accessToken)).toThrow('jwt malformed'); await request(app.getHttpServer()) .put(`/v1/accounts/${address}/storage/counterfactual-safes`) - .set('Cookie', ['access_token=invalid']) + .set('Cookie', [`access_token=${accessToken}`]) .send(createCounterfactualSafeDto) .expect(403); @@ -482,6 +486,7 @@ describe('CounterfactualSafesController', () => { nbf: faker.date.future(), }); + expect(() => jwtService.verify(accessToken)).toThrow('jwt not active'); await request(app.getHttpServer()) .put(`/v1/accounts/${address}/storage/counterfactual-safes`) .set('Cookie', [`access_token=${accessToken}`]) @@ -511,6 +516,7 @@ describe('CounterfactualSafesController', () => { exp: faker.date.past(), }); + expect(() => jwtService.verify(accessToken)).toThrow('jwt expired'); await request(app.getHttpServer()) .put(`/v1/accounts/${address}/storage/counterfactual-safes`) .set('Cookie', [`access_token=${accessToken}`]) @@ -527,7 +533,7 @@ describe('CounterfactualSafesController', () => { }); it('returns 403 if signer_address is not a valid Ethereum address', async () => { - const address = faker.string.hexadecimal() as `0x${string}`; + const address = faker.string.sample() as `0x${string}`; const chain = chainBuilder().build(); const createCounterfactualSafeDto = createCounterfactualSafeDtoBuilder().build(); diff --git a/src/routes/accounts/counterfactual-safes/counterfactual-safes.controller.ts b/src/routes/accounts/counterfactual-safes/counterfactual-safes.controller.ts index 5c5ec51c6d..1017ce0a38 100644 --- a/src/routes/accounts/counterfactual-safes/counterfactual-safes.controller.ts +++ b/src/routes/accounts/counterfactual-safes/counterfactual-safes.controller.ts @@ -38,13 +38,13 @@ export class CounterfactualSafesController { @ApiOkResponse({ type: CounterfactualSafe }) @Put(':address/storage/counterfactual-safes') @UseGuards(AuthGuard) - async getOrCreateCounterfactualSafe( + async upsertCounterfactualSafe( @Auth() authPayload: AuthPayload, @Param('address', new ValidationPipe(AddressSchema)) address: `0x${string}`, @Body(new ValidationPipe(CreateCounterfactualSafeDtoSchema)) createCounterfactualSafeDto: CreateCounterfactualSafeDto, ): Promise { - return this.service.getOrCreateCounterfactualSafe({ + return this.service.upsertCounterfactualSafe({ authPayload, address, createCounterfactualSafeDto, diff --git a/src/routes/accounts/counterfactual-safes/counterfactual-safes.service.ts b/src/routes/accounts/counterfactual-safes/counterfactual-safes.service.ts index bbd8bbc4a6..080b3d13f3 100644 --- a/src/routes/accounts/counterfactual-safes/counterfactual-safes.service.ts +++ b/src/routes/accounts/counterfactual-safes/counterfactual-safes.service.ts @@ -23,13 +23,13 @@ export class CounterfactualSafesService { return this.mapCounterfactualSafe(domainCounterfactualSafe); } - async getOrCreateCounterfactualSafe(args: { + async upsertCounterfactualSafe(args: { authPayload: AuthPayload; address: `0x${string}`; createCounterfactualSafeDto: CreateCounterfactualSafeDto; }): Promise { const domainCounterfactualSafe = - await this.repository.getOrCreateCounterfactualSafe(args); + await this.repository.upsertCounterfactualSafe(args); return this.mapCounterfactualSafe(domainCounterfactualSafe); } @@ -45,7 +45,6 @@ export class CounterfactualSafesService { domainCounterfactualSafe.salt_nonce, domainCounterfactualSafe.singleton_address, domainCounterfactualSafe.threshold, - domainCounterfactualSafe.account_id.toString(), ); } } diff --git a/src/routes/accounts/counterfactual-safes/entities/counterfactual-safe.entity.ts b/src/routes/accounts/counterfactual-safes/entities/counterfactual-safe.entity.ts index 0a731c920c..9de92dd259 100644 --- a/src/routes/accounts/counterfactual-safes/entities/counterfactual-safe.entity.ts +++ b/src/routes/accounts/counterfactual-safes/entities/counterfactual-safe.entity.ts @@ -17,8 +17,6 @@ export class CounterfactualSafe { singletonAddress: `0x${string}`; @ApiProperty() threshold: number; - @ApiProperty() - accountId: string; constructor( chainId: string, @@ -29,7 +27,6 @@ export class CounterfactualSafe { saltNonce: string, singletonAddress: `0x${string}`, threshold: number, - accountId: string, ) { this.chainId = chainId; this.creator = creator; @@ -39,6 +36,5 @@ export class CounterfactualSafe { this.saltNonce = saltNonce; this.singletonAddress = singletonAddress; this.threshold = threshold; - this.accountId = accountId; } }