From 2a0ed4b1484f05785839a7de061b0dbffdf27c84 Mon Sep 17 00:00:00 2001 From: Aaron Cook Date: Thu, 16 Jan 2025 11:29:30 +0100 Subject: [PATCH 1/6] Gracefully handle errors when getting all Safes by owner --- src/domain/safe/safe.repository.ts | 23 +++++-- src/routes/owners/owners.controller.spec.ts | 71 +++++++++++++++++++++ 2 files changed, 87 insertions(+), 7 deletions(-) diff --git a/src/domain/safe/safe.repository.ts b/src/domain/safe/safe.repository.ts index 31d81a0d0e..307e5e7411 100644 --- a/src/domain/safe/safe.repository.ts +++ b/src/domain/safe/safe.repository.ts @@ -427,7 +427,8 @@ export class SafeRepository implements ISafeRepository { ownerAddress: `0x${string}`; }): Promise<{ [chainId: string]: Array }> { const chains = await this.chainsRepository.getAllChains(); - const allSafeLists = await Promise.all( + // Gracefully handle errors in case singular Transaction Service throws + const allSafeLists = await Promise.allSettled( chains.map(async ({ chainId }) => { const safeList = await this.getSafesByOwner({ chainId, @@ -441,12 +442,20 @@ export class SafeRepository implements ISafeRepository { }), ); - return allSafeLists.reduce((acc, { chainId, safeList }) => { - return { - ...acc, - [chainId]: safeList.safes, - }; - }, {}); + const result: { [chainId: string]: Array } = {}; + + for (const [index, allSafeList] of allSafeLists.entries()) { + if (allSafeList.status === 'fulfilled') { + result[allSafeList.value.chainId] = allSafeList.value.safeList.safes; + } else { + const chainId = chains[index].chainId; + this.loggingService.warn( + `Failed to fetch Safe owners. chainId=${chainId}`, + ); + } + } + + return result; } async getLastTransactionSortedByNonce(args: { diff --git a/src/routes/owners/owners.controller.spec.ts b/src/routes/owners/owners.controller.spec.ts index 789dae0b03..4904fa4e6d 100644 --- a/src/routes/owners/owners.controller.spec.ts +++ b/src/routes/owners/owners.controller.spec.ts @@ -33,11 +33,14 @@ import { TestPostgresDatabaseModuleV2 } from '@/datasources/db/v2/test.postgres- import { TestTargetedMessagingDatasourceModule } from '@/datasources/targeted-messaging/__tests__/test.targeted-messaging.datasource.module'; import { TargetedMessagingDatasourceModule } from '@/datasources/targeted-messaging/targeted-messaging.datasource.module'; import { rawify } from '@/validation/entities/raw.entity'; +import { LoggingService } from '@/logging/logging.interface'; +import type { ILoggingService } from '@/logging/logging.interface'; describe('Owners Controller (Unit)', () => { let app: INestApplication; let safeConfigUrl: string; let networkService: jest.MockedObjectDeep; + let loggingService: jest.MockedObjectDeep; beforeEach(async () => { jest.resetAllMocks(); @@ -66,6 +69,7 @@ describe('Owners Controller (Unit)', () => { ); safeConfigUrl = configurationService.getOrThrow('safeConfig.baseUri'); networkService = moduleFixture.get(NetworkService); + loggingService = moduleFixture.get(LoggingService); app = await new TestAppProvider().provide(moduleFixture); await app.init(); @@ -370,6 +374,73 @@ describe('Owners Controller (Unit)', () => { }); }); + it('should gracefully handle chain-specific Transaction Service error', async () => { + jest.spyOn(loggingService, 'warn'); + const ownerAddress = faker.finance.ethereumAddress(); + const chainId1 = faker.string.numeric(); + const chainId2 = faker.string.numeric({ exclude: [chainId1] }); + const chain1 = chainBuilder().with('chainId', chainId1).build(); + const chain2 = chainBuilder().with('chainId', chainId2).build(); + const safesOnChain1 = [ + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + ]; + networkService.get.mockImplementation(({ url }) => { + switch (url) { + case `${safeConfigUrl}/api/v1/chains`: { + return Promise.resolve({ + data: rawify( + pageBuilder() + .with('results', [chain1, chain2]) + .with('next', null) + .build(), + ), + status: 200, + }); + } + case `${safeConfigUrl}/api/v1/chains/${chainId1}`: { + return Promise.resolve({ + data: rawify(chain1), + status: 200, + }); + } + case `${safeConfigUrl}/api/v1/chains/${chainId2}`: { + return Promise.resolve({ + data: rawify(chain2), + status: 200, + }); + } + // ValidationPipe checksums ownerAddress param + case `${chain1.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/`: { + return Promise.resolve({ + data: rawify({ safes: safesOnChain1 }), + status: 200, + }); + } + case `${chain2.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/`: { + return Promise.reject(new Error('Test error')); + } + default: { + return Promise.reject(`No matching rule for url: ${url}`); + } + } + }); + + await request(app.getHttpServer()) + .get(`/v1/owners/${ownerAddress}/safes`) + .expect(200) + .expect({ + // Only "working" chain. Validation schema checksums addresses + [chainId1]: safesOnChain1.map((safe) => getAddress(safe)), + }); + expect(loggingService.warn).toHaveBeenCalledTimes(1); + expect(loggingService.warn).toHaveBeenNthCalledWith( + 1, + `Failed to fetch Safe owners. chainId=${chainId2}`, + ); + }); + it('Failure: Config API fails', async () => { const ownerAddress = faker.finance.ethereumAddress(); From 602a36172bb713810549dcddbce20ed883ec4378 Mon Sep 17 00:00:00 2001 From: Aaron Cook Date: Mon, 20 Jan 2025 09:23:35 +0100 Subject: [PATCH 2/6] Remove comment --- src/domain/safe/safe.repository.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/domain/safe/safe.repository.ts b/src/domain/safe/safe.repository.ts index 307e5e7411..1b3f375d95 100644 --- a/src/domain/safe/safe.repository.ts +++ b/src/domain/safe/safe.repository.ts @@ -427,7 +427,6 @@ export class SafeRepository implements ISafeRepository { ownerAddress: `0x${string}`; }): Promise<{ [chainId: string]: Array }> { const chains = await this.chainsRepository.getAllChains(); - // Gracefully handle errors in case singular Transaction Service throws const allSafeLists = await Promise.allSettled( chains.map(async ({ chainId }) => { const safeList = await this.getSafesByOwner({ From 43c642434ece244e6af023dff5ccfa144fa7d786 Mon Sep 17 00:00:00 2001 From: Aaron Cook Date: Mon, 20 Jan 2025 13:04:28 +0100 Subject: [PATCH 3/6] Add new endpoint that returns `null` --- src/domain/safe/safe.repository.interface.ts | 6 +- src/domain/safe/safe.repository.ts | 43 ++- src/routes/owners/owners.controller.spec.ts | 266 ++++++++++++++++++- src/routes/owners/owners.controller.ts | 38 ++- src/routes/owners/owners.service.ts | 8 +- 5 files changed, 342 insertions(+), 19 deletions(-) diff --git a/src/domain/safe/safe.repository.interface.ts b/src/domain/safe/safe.repository.interface.ts index dcb488c6e6..3a9a528e6b 100644 --- a/src/domain/safe/safe.repository.interface.ts +++ b/src/domain/safe/safe.repository.interface.ts @@ -175,10 +175,14 @@ export interface ISafeRepository { ownerAddress: `0x${string}`; }): Promise; - getAllSafesByOwner(args: { + deprecated__getAllSafesByOwner(args: { ownerAddress: `0x${string}`; }): Promise<{ [chainId: string]: Array }>; + getAllSafesByOwner(args: { + ownerAddress: `0x${string}`; + }): Promise<{ [chainId: string]: Array | null }>; + getLastTransactionSortedByNonce(args: { chainId: string; safeAddress: `0x${string}`; diff --git a/src/domain/safe/safe.repository.ts b/src/domain/safe/safe.repository.ts index 1b3f375d95..601b927f47 100644 --- a/src/domain/safe/safe.repository.ts +++ b/src/domain/safe/safe.repository.ts @@ -423,9 +423,35 @@ export class SafeRepository implements ISafeRepository { return SafeListSchema.parse(safeList); } - async getAllSafesByOwner(args: { + async deprecated__getAllSafesByOwner(args: { ownerAddress: `0x${string}`; }): Promise<{ [chainId: string]: Array }> { + const chains = await this.chainsRepository.getAllChains(); + const allSafeLists = await Promise.all( + chains.map(async ({ chainId }) => { + const safeList = await this.getSafesByOwner({ + chainId, + ownerAddress: args.ownerAddress, + }); + + return { + chainId, + safeList, + }; + }), + ); + + return allSafeLists.reduce((acc, { chainId, safeList }) => { + return { + ...acc, + [chainId]: safeList.safes, + }; + }, {}); + } + + async getAllSafesByOwner(args: { + ownerAddress: `0x${string}`; + }): Promise<{ [chainId: string]: Array | null }> { const chains = await this.chainsRepository.getAllChains(); const allSafeLists = await Promise.allSettled( chains.map(async ({ chainId }) => { @@ -441,17 +467,22 @@ export class SafeRepository implements ISafeRepository { }), ); - const result: { [chainId: string]: Array } = {}; + const result: { [chainId: string]: Array | null } = {}; for (const [index, allSafeList] of allSafeLists.entries()) { - if (allSafeList.status === 'fulfilled') { - result[allSafeList.value.chainId] = allSafeList.value.safeList.safes; - } else { - const chainId = chains[index].chainId; + const chainId = chains[index].chainId; + + if (allSafeList.status === 'rejected') { this.loggingService.warn( `Failed to fetch Safe owners. chainId=${chainId}`, ); } + + result[chainId] = + allSafeList.status === 'fulfilled' + ? allSafeList.value.safeList.safes + : // Transaction Service threw; could own Safes or not + null; } return result; diff --git a/src/routes/owners/owners.controller.spec.ts b/src/routes/owners/owners.controller.spec.ts index 4904fa4e6d..03b8d605ad 100644 --- a/src/routes/owners/owners.controller.spec.ts +++ b/src/routes/owners/owners.controller.spec.ts @@ -202,7 +202,7 @@ describe('Owners Controller (Unit)', () => { }); }); - describe('GET all safes by owner address', () => { + describe('deprecated: GET all safes by owner address', () => { it('Success for singular chain page', async () => { const ownerAddress = faker.finance.ethereumAddress(); @@ -374,6 +374,262 @@ describe('Owners Controller (Unit)', () => { }); }); + it('Failure: Config API fails', async () => { + const ownerAddress = faker.finance.ethereumAddress(); + + networkService.get.mockImplementation(({ url }) => { + if (url === `${safeConfigUrl}/api/v1/chains`) { + const error = new NetworkResponseError( + new URL(`${safeConfigUrl}/api/v1/chains`), + { + status: 500, + } as Response, + ); + return Promise.reject(error); + } + return Promise.reject(`No matching rule for url: ${url}`); + }); + + await request(app.getHttpServer()) + .get(`/v1/owners/${ownerAddress}/safes`) + .expect(500) + .expect({ + message: 'An error occurred', + code: 500, + }); + + expect(networkService.get).toHaveBeenCalledTimes(1); + expect(networkService.get).toHaveBeenCalledWith({ + url: `${safeConfigUrl}/api/v1/chains`, + networkRequest: { + params: { limit: ChainsRepository.MAX_LIMIT, offset: 0 }, + }, + }); + }); + + it('Failure: data validation fails', async () => { + const ownerAddress = faker.finance.ethereumAddress(); + + const chainId = faker.string.numeric(); + + const chain = chainBuilder().with('chainId', chainId).build(); + + const safesOnChain = [ + faker.finance.ethereumAddress(), + faker.number.int(), + faker.finance.ethereumAddress(), + ]; + + networkService.get.mockImplementation(({ url }) => { + switch (url) { + case `${safeConfigUrl}/api/v1/chains`: { + return Promise.resolve({ + data: rawify({ + results: [chain], + }), + status: 200, + }); + } + + case `${safeConfigUrl}/api/v1/chains/${chainId}`: { + return Promise.resolve({ + data: rawify(chain), + status: 200, + }); + } + + case `${chain.transactionService}/api/v1/owners/${ownerAddress}/safes/`: { + return Promise.resolve({ + data: rawify({ safes: safesOnChain }), + status: 200, + }); + } + + default: { + return Promise.reject(`No matching rule for url: ${url}`); + } + } + }); + + await request(app.getHttpServer()) + .get(`/v1/owners/${ownerAddress}/safes`) + .expect(502) + .expect({ statusCode: 502, message: 'Bad gateway' }); + }); + }); + + describe('GET all safes by owner address', () => { + it('Success for singular chain page', async () => { + const ownerAddress = faker.finance.ethereumAddress(); + + const chainId1 = faker.string.numeric(); + const chainId2 = faker.string.numeric({ exclude: [chainId1] }); + + const chain1 = chainBuilder().with('chainId', chainId1).build(); + const chain2 = chainBuilder().with('chainId', chainId2).build(); + + const safesOnChain1 = [ + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + ]; + const safesOnChain2 = [ + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + ]; + + networkService.get.mockImplementation(({ url }) => { + switch (url) { + case `${safeConfigUrl}/api/v1/chains`: { + return Promise.resolve({ + data: rawify( + pageBuilder() + .with('results', [chain1, chain2]) + .with('next', null) + .build(), + ), + status: 200, + }); + } + + case `${safeConfigUrl}/api/v1/chains/${chainId1}`: { + return Promise.resolve({ + data: rawify(chain1), + status: 200, + }); + } + + case `${safeConfigUrl}/api/v1/chains/${chainId2}`: { + return Promise.resolve({ + data: rawify(chain2), + status: 200, + }); + } + + // ValidationPipe checksums ownerAddress param + case `${chain1.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/`: { + return Promise.resolve({ + data: rawify({ safes: safesOnChain1 }), + status: 200, + }); + } + + case `${chain2.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/`: { + return Promise.resolve({ + data: rawify({ safes: safesOnChain2 }), + status: 200, + }); + } + + default: { + return Promise.reject(`No matching rule for url: ${url}`); + } + } + }); + + await request(app.getHttpServer()) + .get(`/v2/owners/${ownerAddress}/safes`) + .expect(200) + .expect({ + // Validation schema checksums addresses + [chainId1]: safesOnChain1.map((safe) => getAddress(safe)), + [chainId2]: safesOnChain2.map((safe) => getAddress(safe)), + }); + }); + + it('Success for multiple chain pages', async () => { + const ownerAddress = faker.finance.ethereumAddress(); + + const chainId1 = faker.string.numeric(); + const chainId2 = faker.string.numeric({ exclude: [chainId1] }); + + const chain1 = chainBuilder().with('chainId', chainId1).build(); + const chain2 = chainBuilder().with('chainId', chainId2).build(); + + const chainsUrl = `${safeConfigUrl}/api/v1/chains`; + const offset = 1; + const chainsPage1 = pageBuilder() + .with('results', [chain1]) + .with('next', limitAndOffsetUrlFactory(undefined, offset, chainsUrl)) + .build(); + const chainsPage2 = pageBuilder() + .with('results', [chain2]) + .with('next', null) + .build(); + + const safesOnChain1 = [ + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + ]; + const safesOnChain2 = [ + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + ]; + + networkService.get.mockImplementation(({ url, networkRequest }) => { + if (url === chainsUrl && !networkRequest!.params!.offset) { + return Promise.resolve({ + data: rawify(chainsPage1), + status: 200, + }); + } + if (url === chainsUrl && networkRequest!.params!.offset === offset) { + return Promise.resolve({ + data: rawify(chainsPage2), + status: 200, + }); + } + if (url === `${safeConfigUrl}/api/v1/chains/${chainId1}`) { + return Promise.resolve({ + data: rawify(chain1), + status: 200, + }); + } + + if (url === `${safeConfigUrl}/api/v1/chains/${chainId2}`) { + return Promise.resolve({ + data: rawify(chain2), + status: 200, + }); + } + + // ValidationPipe checksums ownerAddress param + if ( + url === + `${chain1.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/` + ) { + return Promise.resolve({ + data: rawify({ safes: safesOnChain1 }), + status: 200, + }); + } + + if ( + url === + `${chain2.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/` + ) { + return Promise.resolve({ + data: rawify({ safes: safesOnChain2 }), + status: 200, + }); + } + + return Promise.reject(`No matching rule for url: ${url}`); + }); + + await request(app.getHttpServer()) + .get(`/v2/owners/${ownerAddress}/safes`) + .expect(200) + .expect({ + // Validation schema checksums addresses + [chainId1]: safesOnChain1.map((safe) => getAddress(safe)), + [chainId2]: safesOnChain2.map((safe) => getAddress(safe)), + }); + }); + it('should gracefully handle chain-specific Transaction Service error', async () => { jest.spyOn(loggingService, 'warn'); const ownerAddress = faker.finance.ethereumAddress(); @@ -428,11 +684,11 @@ describe('Owners Controller (Unit)', () => { }); await request(app.getHttpServer()) - .get(`/v1/owners/${ownerAddress}/safes`) + .get(`/v2/owners/${ownerAddress}/safes`) .expect(200) .expect({ - // Only "working" chain. Validation schema checksums addresses [chainId1]: safesOnChain1.map((safe) => getAddress(safe)), + [chainId2]: null, }); expect(loggingService.warn).toHaveBeenCalledTimes(1); expect(loggingService.warn).toHaveBeenNthCalledWith( @@ -458,7 +714,7 @@ describe('Owners Controller (Unit)', () => { }); await request(app.getHttpServer()) - .get(`/v1/owners/${ownerAddress}/safes`) + .get(`/v2/owners/${ownerAddress}/safes`) .expect(500) .expect({ message: 'An error occurred', @@ -519,7 +775,7 @@ describe('Owners Controller (Unit)', () => { }); await request(app.getHttpServer()) - .get(`/v1/owners/${ownerAddress}/safes`) + .get(`/v2/owners/${ownerAddress}/safes`) .expect(502) .expect({ statusCode: 502, message: 'Bad gateway' }); }); diff --git a/src/routes/owners/owners.controller.ts b/src/routes/owners/owners.controller.ts index c57e89545f..b920de98a3 100644 --- a/src/routes/owners/owners.controller.ts +++ b/src/routes/owners/owners.controller.ts @@ -1,5 +1,5 @@ import { Controller, Get, Param } from '@nestjs/common'; -import { ApiOkResponse, ApiTags } from '@nestjs/swagger'; +import { ApiOkResponse, ApiOperation, ApiTags } from '@nestjs/swagger'; import { SafeList } from '@/routes/owners/entities/safe-list.entity'; import { OwnersService } from '@/routes/owners/owners.service'; import { ValidationPipe } from '@/validation/pipes/validation.pipe'; @@ -8,13 +8,12 @@ import { AddressSchema } from '@/validation/entities/schemas/address.schema'; @ApiTags('owners') @Controller({ path: '', - version: '1', }) export class OwnersController { constructor(private readonly ownersService: OwnersService) {} @ApiOkResponse({ type: SafeList }) - @Get('chains/:chainId/owners/:ownerAddress/safes') + @Get('/v1/chains/:chainId/owners/:ownerAddress/safes') async getSafesByOwner( @Param('chainId') chainId: string, @Param('ownerAddress', new ValidationPipe(AddressSchema)) @@ -23,12 +22,39 @@ export class OwnersController { return this.ownersService.getSafesByOwner({ chainId, ownerAddress }); } - @ApiOkResponse({ type: SafeList }) - @Get('owners/:ownerAddress/safes') - async getAllSafesByOwner( + @ApiOkResponse({ + schema: { + type: 'object', + additionalProperties: { + type: 'array', + items: { type: 'string' }, + }, + }, + }) + @ApiOperation({ deprecated: true }) + @Get('/v1/owners/:ownerAddress/safes') + async deprecated__getAllSafesByOwner( @Param('ownerAddress', new ValidationPipe(AddressSchema)) ownerAddress: `0x${string}`, ): Promise<{ [chainId: string]: Array }> { + return this.ownersService.deprecated__getAllSafesByOwner({ ownerAddress }); + } + + @ApiOkResponse({ + schema: { + type: 'object', + additionalProperties: { + type: 'array', + items: { type: 'string' }, + nullable: true, + }, + }, + }) + @Get('/v2/owners/:ownerAddress/safes') + async getAllSafesByOwner( + @Param('ownerAddress', new ValidationPipe(AddressSchema)) + ownerAddress: `0x${string}`, + ): Promise<{ [chainId: string]: Array | null }> { return this.ownersService.getAllSafesByOwner({ ownerAddress }); } } diff --git a/src/routes/owners/owners.service.ts b/src/routes/owners/owners.service.ts index 28b4fef921..9fa12c60a7 100644 --- a/src/routes/owners/owners.service.ts +++ b/src/routes/owners/owners.service.ts @@ -17,9 +17,15 @@ export class OwnersService { return this.safeRepository.getSafesByOwner(args); } - async getAllSafesByOwner(args: { + async deprecated__getAllSafesByOwner(args: { ownerAddress: `0x${string}`; }): Promise<{ [chainId: string]: Array }> { + return this.safeRepository.deprecated__getAllSafesByOwner(args); + } + + async getAllSafesByOwner(args: { + ownerAddress: `0x${string}`; + }): Promise<{ [chainId: string]: Array | null }> { return this.safeRepository.getAllSafesByOwner(args); } } From 6055f72f24e84bb2123107ab62017047d18f2fa2 Mon Sep 17 00:00:00 2001 From: Aaron Cook Date: Mon, 20 Jan 2025 13:20:39 +0100 Subject: [PATCH 4/6] Fix E2E test --- src/routes/owners/__tests__/get-safes-by-owner.e2e-spec.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/routes/owners/__tests__/get-safes-by-owner.e2e-spec.ts b/src/routes/owners/__tests__/get-safes-by-owner.e2e-spec.ts index ab5c2af70b..0f82e3abe8 100644 --- a/src/routes/owners/__tests__/get-safes-by-owner.e2e-spec.ts +++ b/src/routes/owners/__tests__/get-safes-by-owner.e2e-spec.ts @@ -47,11 +47,13 @@ describe('Get safes by owner e2e test', () => { await redisClient.quit(); }); - it('GET /owners//safes', async () => { + it('GET /v1/chains//owners//safes', async () => { const ownerCacheKey = `${cacheKeyPrefix}-${TEST_SAFE.chainId}_owner_safes_${TEST_SAFE.owners[0]}`; await request(app.getHttpServer()) - .get(`/chains/${TEST_SAFE.chainId}/owners/${TEST_SAFE.owners[0]}/safes`) + .get( + `/v1/chains/${TEST_SAFE.chainId}/owners/${TEST_SAFE.owners[0]}/safes`, + ) .expect(200) .then(({ body }) => { expect(body).toEqual( From 2e27da02514d37577621737f2a9d10968172e559 Mon Sep 17 00:00:00 2001 From: Aaron Cook Date: Mon, 20 Jan 2025 13:55:53 +0100 Subject: [PATCH 5/6] Only check success once --- src/domain/safe/safe.repository.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/domain/safe/safe.repository.ts b/src/domain/safe/safe.repository.ts index 601b927f47..fdc98090c3 100644 --- a/src/domain/safe/safe.repository.ts +++ b/src/domain/safe/safe.repository.ts @@ -472,17 +472,14 @@ export class SafeRepository implements ISafeRepository { for (const [index, allSafeList] of allSafeLists.entries()) { const chainId = chains[index].chainId; - if (allSafeList.status === 'rejected') { + if (allSafeList.status === 'fulfilled') { + result[chainId] = allSafeList.value.safeList.safes; + } else { + result[chainId] = null; this.loggingService.warn( `Failed to fetch Safe owners. chainId=${chainId}`, ); } - - result[chainId] = - allSafeList.status === 'fulfilled' - ? allSafeList.value.safeList.safes - : // Transaction Service threw; could own Safes or not - null; } return result; From 912117016c32895b9bd01a41c8ac3a7730ead883 Mon Sep 17 00:00:00 2001 From: Aaron Cook Date: Mon, 20 Jan 2025 15:22:44 +0100 Subject: [PATCH 6/6] Split controller --- src/domain/safe/safe.repository.ts | 2 + .../__tests__/get-safes-by-owner.e2e-spec.ts | 6 +- ...r.spec.ts => owners.controller.v1.spec.ts} | 329 +------------- ....controller.ts => owners.controller.v1.ts} | 40 +- .../owners/owners.controller.v2.spec.ts | 406 ++++++++++++++++++ src/routes/owners/owners.controller.v2.ts | 31 ++ src/routes/owners/owners.module.ts | 5 +- src/routes/owners/owners.service.ts | 2 + 8 files changed, 454 insertions(+), 367 deletions(-) rename src/routes/owners/{owners.controller.spec.ts => owners.controller.v1.spec.ts} (59%) rename src/routes/owners/{owners.controller.ts => owners.controller.v1.ts} (52%) create mode 100644 src/routes/owners/owners.controller.v2.spec.ts create mode 100644 src/routes/owners/owners.controller.v2.ts diff --git a/src/domain/safe/safe.repository.ts b/src/domain/safe/safe.repository.ts index fdc98090c3..ee06417258 100644 --- a/src/domain/safe/safe.repository.ts +++ b/src/domain/safe/safe.repository.ts @@ -423,6 +423,8 @@ export class SafeRepository implements ISafeRepository { return SafeListSchema.parse(safeList); } + // TODO: Remove with /owners/:ownerAddress/safes + // @deprecated async deprecated__getAllSafesByOwner(args: { ownerAddress: `0x${string}`; }): Promise<{ [chainId: string]: Array }> { diff --git a/src/routes/owners/__tests__/get-safes-by-owner.e2e-spec.ts b/src/routes/owners/__tests__/get-safes-by-owner.e2e-spec.ts index 0f82e3abe8..ab5c2af70b 100644 --- a/src/routes/owners/__tests__/get-safes-by-owner.e2e-spec.ts +++ b/src/routes/owners/__tests__/get-safes-by-owner.e2e-spec.ts @@ -47,13 +47,11 @@ describe('Get safes by owner e2e test', () => { await redisClient.quit(); }); - it('GET /v1/chains//owners//safes', async () => { + it('GET /owners//safes', async () => { const ownerCacheKey = `${cacheKeyPrefix}-${TEST_SAFE.chainId}_owner_safes_${TEST_SAFE.owners[0]}`; await request(app.getHttpServer()) - .get( - `/v1/chains/${TEST_SAFE.chainId}/owners/${TEST_SAFE.owners[0]}/safes`, - ) + .get(`/chains/${TEST_SAFE.chainId}/owners/${TEST_SAFE.owners[0]}/safes`) .expect(200) .then(({ body }) => { expect(body).toEqual( diff --git a/src/routes/owners/owners.controller.spec.ts b/src/routes/owners/owners.controller.v1.spec.ts similarity index 59% rename from src/routes/owners/owners.controller.spec.ts rename to src/routes/owners/owners.controller.v1.spec.ts index 03b8d605ad..789dae0b03 100644 --- a/src/routes/owners/owners.controller.spec.ts +++ b/src/routes/owners/owners.controller.v1.spec.ts @@ -33,14 +33,11 @@ import { TestPostgresDatabaseModuleV2 } from '@/datasources/db/v2/test.postgres- import { TestTargetedMessagingDatasourceModule } from '@/datasources/targeted-messaging/__tests__/test.targeted-messaging.datasource.module'; import { TargetedMessagingDatasourceModule } from '@/datasources/targeted-messaging/targeted-messaging.datasource.module'; import { rawify } from '@/validation/entities/raw.entity'; -import { LoggingService } from '@/logging/logging.interface'; -import type { ILoggingService } from '@/logging/logging.interface'; describe('Owners Controller (Unit)', () => { let app: INestApplication; let safeConfigUrl: string; let networkService: jest.MockedObjectDeep; - let loggingService: jest.MockedObjectDeep; beforeEach(async () => { jest.resetAllMocks(); @@ -69,7 +66,6 @@ describe('Owners Controller (Unit)', () => { ); safeConfigUrl = configurationService.getOrThrow('safeConfig.baseUri'); networkService = moduleFixture.get(NetworkService); - loggingService = moduleFixture.get(LoggingService); app = await new TestAppProvider().provide(moduleFixture); await app.init(); @@ -202,7 +198,7 @@ describe('Owners Controller (Unit)', () => { }); }); - describe('deprecated: GET all safes by owner address', () => { + describe('GET all safes by owner address', () => { it('Success for singular chain page', async () => { const ownerAddress = faker.finance.ethereumAddress(); @@ -457,327 +453,4 @@ describe('Owners Controller (Unit)', () => { .expect({ statusCode: 502, message: 'Bad gateway' }); }); }); - - describe('GET all safes by owner address', () => { - it('Success for singular chain page', async () => { - const ownerAddress = faker.finance.ethereumAddress(); - - const chainId1 = faker.string.numeric(); - const chainId2 = faker.string.numeric({ exclude: [chainId1] }); - - const chain1 = chainBuilder().with('chainId', chainId1).build(); - const chain2 = chainBuilder().with('chainId', chainId2).build(); - - const safesOnChain1 = [ - faker.finance.ethereumAddress(), - faker.finance.ethereumAddress(), - faker.finance.ethereumAddress(), - ]; - const safesOnChain2 = [ - faker.finance.ethereumAddress(), - faker.finance.ethereumAddress(), - faker.finance.ethereumAddress(), - ]; - - networkService.get.mockImplementation(({ url }) => { - switch (url) { - case `${safeConfigUrl}/api/v1/chains`: { - return Promise.resolve({ - data: rawify( - pageBuilder() - .with('results', [chain1, chain2]) - .with('next', null) - .build(), - ), - status: 200, - }); - } - - case `${safeConfigUrl}/api/v1/chains/${chainId1}`: { - return Promise.resolve({ - data: rawify(chain1), - status: 200, - }); - } - - case `${safeConfigUrl}/api/v1/chains/${chainId2}`: { - return Promise.resolve({ - data: rawify(chain2), - status: 200, - }); - } - - // ValidationPipe checksums ownerAddress param - case `${chain1.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/`: { - return Promise.resolve({ - data: rawify({ safes: safesOnChain1 }), - status: 200, - }); - } - - case `${chain2.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/`: { - return Promise.resolve({ - data: rawify({ safes: safesOnChain2 }), - status: 200, - }); - } - - default: { - return Promise.reject(`No matching rule for url: ${url}`); - } - } - }); - - await request(app.getHttpServer()) - .get(`/v2/owners/${ownerAddress}/safes`) - .expect(200) - .expect({ - // Validation schema checksums addresses - [chainId1]: safesOnChain1.map((safe) => getAddress(safe)), - [chainId2]: safesOnChain2.map((safe) => getAddress(safe)), - }); - }); - - it('Success for multiple chain pages', async () => { - const ownerAddress = faker.finance.ethereumAddress(); - - const chainId1 = faker.string.numeric(); - const chainId2 = faker.string.numeric({ exclude: [chainId1] }); - - const chain1 = chainBuilder().with('chainId', chainId1).build(); - const chain2 = chainBuilder().with('chainId', chainId2).build(); - - const chainsUrl = `${safeConfigUrl}/api/v1/chains`; - const offset = 1; - const chainsPage1 = pageBuilder() - .with('results', [chain1]) - .with('next', limitAndOffsetUrlFactory(undefined, offset, chainsUrl)) - .build(); - const chainsPage2 = pageBuilder() - .with('results', [chain2]) - .with('next', null) - .build(); - - const safesOnChain1 = [ - faker.finance.ethereumAddress(), - faker.finance.ethereumAddress(), - faker.finance.ethereumAddress(), - ]; - const safesOnChain2 = [ - faker.finance.ethereumAddress(), - faker.finance.ethereumAddress(), - faker.finance.ethereumAddress(), - ]; - - networkService.get.mockImplementation(({ url, networkRequest }) => { - if (url === chainsUrl && !networkRequest!.params!.offset) { - return Promise.resolve({ - data: rawify(chainsPage1), - status: 200, - }); - } - if (url === chainsUrl && networkRequest!.params!.offset === offset) { - return Promise.resolve({ - data: rawify(chainsPage2), - status: 200, - }); - } - if (url === `${safeConfigUrl}/api/v1/chains/${chainId1}`) { - return Promise.resolve({ - data: rawify(chain1), - status: 200, - }); - } - - if (url === `${safeConfigUrl}/api/v1/chains/${chainId2}`) { - return Promise.resolve({ - data: rawify(chain2), - status: 200, - }); - } - - // ValidationPipe checksums ownerAddress param - if ( - url === - `${chain1.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/` - ) { - return Promise.resolve({ - data: rawify({ safes: safesOnChain1 }), - status: 200, - }); - } - - if ( - url === - `${chain2.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/` - ) { - return Promise.resolve({ - data: rawify({ safes: safesOnChain2 }), - status: 200, - }); - } - - return Promise.reject(`No matching rule for url: ${url}`); - }); - - await request(app.getHttpServer()) - .get(`/v2/owners/${ownerAddress}/safes`) - .expect(200) - .expect({ - // Validation schema checksums addresses - [chainId1]: safesOnChain1.map((safe) => getAddress(safe)), - [chainId2]: safesOnChain2.map((safe) => getAddress(safe)), - }); - }); - - it('should gracefully handle chain-specific Transaction Service error', async () => { - jest.spyOn(loggingService, 'warn'); - const ownerAddress = faker.finance.ethereumAddress(); - const chainId1 = faker.string.numeric(); - const chainId2 = faker.string.numeric({ exclude: [chainId1] }); - const chain1 = chainBuilder().with('chainId', chainId1).build(); - const chain2 = chainBuilder().with('chainId', chainId2).build(); - const safesOnChain1 = [ - faker.finance.ethereumAddress(), - faker.finance.ethereumAddress(), - faker.finance.ethereumAddress(), - ]; - networkService.get.mockImplementation(({ url }) => { - switch (url) { - case `${safeConfigUrl}/api/v1/chains`: { - return Promise.resolve({ - data: rawify( - pageBuilder() - .with('results', [chain1, chain2]) - .with('next', null) - .build(), - ), - status: 200, - }); - } - case `${safeConfigUrl}/api/v1/chains/${chainId1}`: { - return Promise.resolve({ - data: rawify(chain1), - status: 200, - }); - } - case `${safeConfigUrl}/api/v1/chains/${chainId2}`: { - return Promise.resolve({ - data: rawify(chain2), - status: 200, - }); - } - // ValidationPipe checksums ownerAddress param - case `${chain1.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/`: { - return Promise.resolve({ - data: rawify({ safes: safesOnChain1 }), - status: 200, - }); - } - case `${chain2.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/`: { - return Promise.reject(new Error('Test error')); - } - default: { - return Promise.reject(`No matching rule for url: ${url}`); - } - } - }); - - await request(app.getHttpServer()) - .get(`/v2/owners/${ownerAddress}/safes`) - .expect(200) - .expect({ - [chainId1]: safesOnChain1.map((safe) => getAddress(safe)), - [chainId2]: null, - }); - expect(loggingService.warn).toHaveBeenCalledTimes(1); - expect(loggingService.warn).toHaveBeenNthCalledWith( - 1, - `Failed to fetch Safe owners. chainId=${chainId2}`, - ); - }); - - it('Failure: Config API fails', async () => { - const ownerAddress = faker.finance.ethereumAddress(); - - networkService.get.mockImplementation(({ url }) => { - if (url === `${safeConfigUrl}/api/v1/chains`) { - const error = new NetworkResponseError( - new URL(`${safeConfigUrl}/api/v1/chains`), - { - status: 500, - } as Response, - ); - return Promise.reject(error); - } - return Promise.reject(`No matching rule for url: ${url}`); - }); - - await request(app.getHttpServer()) - .get(`/v2/owners/${ownerAddress}/safes`) - .expect(500) - .expect({ - message: 'An error occurred', - code: 500, - }); - - expect(networkService.get).toHaveBeenCalledTimes(1); - expect(networkService.get).toHaveBeenCalledWith({ - url: `${safeConfigUrl}/api/v1/chains`, - networkRequest: { - params: { limit: ChainsRepository.MAX_LIMIT, offset: 0 }, - }, - }); - }); - - it('Failure: data validation fails', async () => { - const ownerAddress = faker.finance.ethereumAddress(); - - const chainId = faker.string.numeric(); - - const chain = chainBuilder().with('chainId', chainId).build(); - - const safesOnChain = [ - faker.finance.ethereumAddress(), - faker.number.int(), - faker.finance.ethereumAddress(), - ]; - - networkService.get.mockImplementation(({ url }) => { - switch (url) { - case `${safeConfigUrl}/api/v1/chains`: { - return Promise.resolve({ - data: rawify({ - results: [chain], - }), - status: 200, - }); - } - - case `${safeConfigUrl}/api/v1/chains/${chainId}`: { - return Promise.resolve({ - data: rawify(chain), - status: 200, - }); - } - - case `${chain.transactionService}/api/v1/owners/${ownerAddress}/safes/`: { - return Promise.resolve({ - data: rawify({ safes: safesOnChain }), - status: 200, - }); - } - - default: { - return Promise.reject(`No matching rule for url: ${url}`); - } - } - }); - - await request(app.getHttpServer()) - .get(`/v2/owners/${ownerAddress}/safes`) - .expect(502) - .expect({ statusCode: 502, message: 'Bad gateway' }); - }); - }); }); diff --git a/src/routes/owners/owners.controller.ts b/src/routes/owners/owners.controller.v1.ts similarity index 52% rename from src/routes/owners/owners.controller.ts rename to src/routes/owners/owners.controller.v1.ts index b920de98a3..1c9d8ac80e 100644 --- a/src/routes/owners/owners.controller.ts +++ b/src/routes/owners/owners.controller.v1.ts @@ -1,5 +1,5 @@ import { Controller, Get, Param } from '@nestjs/common'; -import { ApiOkResponse, ApiOperation, ApiTags } from '@nestjs/swagger'; +import { ApiOkResponse, ApiTags } from '@nestjs/swagger'; import { SafeList } from '@/routes/owners/entities/safe-list.entity'; import { OwnersService } from '@/routes/owners/owners.service'; import { ValidationPipe } from '@/validation/pipes/validation.pipe'; @@ -8,12 +8,13 @@ import { AddressSchema } from '@/validation/entities/schemas/address.schema'; @ApiTags('owners') @Controller({ path: '', + version: '1', }) -export class OwnersController { +export class OwnersControllerV1 { constructor(private readonly ownersService: OwnersService) {} @ApiOkResponse({ type: SafeList }) - @Get('/v1/chains/:chainId/owners/:ownerAddress/safes') + @Get('chains/:chainId/owners/:ownerAddress/safes') async getSafesByOwner( @Param('chainId') chainId: string, @Param('ownerAddress', new ValidationPipe(AddressSchema)) @@ -22,39 +23,12 @@ export class OwnersController { return this.ownersService.getSafesByOwner({ chainId, ownerAddress }); } - @ApiOkResponse({ - schema: { - type: 'object', - additionalProperties: { - type: 'array', - items: { type: 'string' }, - }, - }, - }) - @ApiOperation({ deprecated: true }) - @Get('/v1/owners/:ownerAddress/safes') - async deprecated__getAllSafesByOwner( + @ApiOkResponse({ type: SafeList }) + @Get('owners/:ownerAddress/safes') + async getAllSafesByOwner( @Param('ownerAddress', new ValidationPipe(AddressSchema)) ownerAddress: `0x${string}`, ): Promise<{ [chainId: string]: Array }> { return this.ownersService.deprecated__getAllSafesByOwner({ ownerAddress }); } - - @ApiOkResponse({ - schema: { - type: 'object', - additionalProperties: { - type: 'array', - items: { type: 'string' }, - nullable: true, - }, - }, - }) - @Get('/v2/owners/:ownerAddress/safes') - async getAllSafesByOwner( - @Param('ownerAddress', new ValidationPipe(AddressSchema)) - ownerAddress: `0x${string}`, - ): Promise<{ [chainId: string]: Array | null }> { - return this.ownersService.getAllSafesByOwner({ ownerAddress }); - } } diff --git a/src/routes/owners/owners.controller.v2.spec.ts b/src/routes/owners/owners.controller.v2.spec.ts new file mode 100644 index 0000000000..51734cb3dd --- /dev/null +++ b/src/routes/owners/owners.controller.v2.spec.ts @@ -0,0 +1,406 @@ +import { faker } from '@faker-js/faker'; +import type { INestApplication } from '@nestjs/common'; +import type { TestingModule } from '@nestjs/testing'; +import { Test } from '@nestjs/testing'; +import request from 'supertest'; +import { TestAppProvider } from '@/__tests__/test-app.provider'; +import { TestCacheModule } from '@/datasources/cache/__tests__/test.cache.module'; +import { TestNetworkModule } from '@/datasources/network/__tests__/test.network.module'; +import { chainBuilder } from '@/domain/chains/entities/__tests__/chain.builder'; +import { TestLoggingModule } from '@/logging/__tests__/test.logging.module'; +import configuration from '@/config/entities/__tests__/configuration'; +import { IConfigurationService } from '@/config/configuration.service.interface'; +import type { INetworkService } from '@/datasources/network/network.service.interface'; +import { NetworkService } from '@/datasources/network/network.service.interface'; +import { AppModule } from '@/app.module'; +import { CacheModule } from '@/datasources/cache/cache.module'; +import { RequestScopedLoggingModule } from '@/logging/logging.module'; +import { NetworkModule } from '@/datasources/network/network.module'; +import { NetworkResponseError } from '@/datasources/network/entities/network.error.entity'; +import { getAddress } from 'viem'; +import { + limitAndOffsetUrlFactory, + pageBuilder, +} from '@/domain/entities/__tests__/page.builder'; +import { TestQueuesApiModule } from '@/datasources/queues/__tests__/test.queues-api.module'; +import { QueuesApiModule } from '@/datasources/queues/queues-api.module'; +import type { Server } from 'net'; +import { TestPostgresDatabaseModule } from '@/datasources/db/__tests__/test.postgres-database.module'; +import { PostgresDatabaseModule } from '@/datasources/db/v1/postgres-database.module'; +import { ChainsRepository } from '@/domain/chains/chains.repository'; +import { PostgresDatabaseModuleV2 } from '@/datasources/db/v2/postgres-database.module'; +import { TestPostgresDatabaseModuleV2 } from '@/datasources/db/v2/test.postgres-database.module'; +import { TestTargetedMessagingDatasourceModule } from '@/datasources/targeted-messaging/__tests__/test.targeted-messaging.datasource.module'; +import { TargetedMessagingDatasourceModule } from '@/datasources/targeted-messaging/targeted-messaging.datasource.module'; +import { rawify } from '@/validation/entities/raw.entity'; +import { + type ILoggingService, + LoggingService, +} from '@/logging/logging.interface'; + +describe('Owners Controller (Unit)', () => { + let app: INestApplication; + let safeConfigUrl: string; + let networkService: jest.MockedObjectDeep; + let loggingService: jest.MockedObjectDeep; + + beforeEach(async () => { + jest.resetAllMocks(); + + const moduleFixture: TestingModule = await Test.createTestingModule({ + imports: [AppModule.register(configuration)], + }) + .overrideModule(PostgresDatabaseModule) + .useModule(TestPostgresDatabaseModule) + .overrideModule(TargetedMessagingDatasourceModule) + .useModule(TestTargetedMessagingDatasourceModule) + .overrideModule(CacheModule) + .useModule(TestCacheModule) + .overrideModule(RequestScopedLoggingModule) + .useModule(TestLoggingModule) + .overrideModule(NetworkModule) + .useModule(TestNetworkModule) + .overrideModule(QueuesApiModule) + .useModule(TestQueuesApiModule) + .overrideModule(PostgresDatabaseModuleV2) + .useModule(TestPostgresDatabaseModuleV2) + .compile(); + + const configurationService = moduleFixture.get( + IConfigurationService, + ); + safeConfigUrl = configurationService.getOrThrow('safeConfig.baseUri'); + networkService = moduleFixture.get(NetworkService); + loggingService = moduleFixture.get(LoggingService); + + app = await new TestAppProvider().provide(moduleFixture); + await app.init(); + }); + + afterAll(async () => { + await app.close(); + }); + + describe('GET all safes by owner address', () => { + it('Success for singular chain page', async () => { + const ownerAddress = faker.finance.ethereumAddress(); + + const chainId1 = faker.string.numeric(); + const chainId2 = faker.string.numeric({ exclude: [chainId1] }); + + const chain1 = chainBuilder().with('chainId', chainId1).build(); + const chain2 = chainBuilder().with('chainId', chainId2).build(); + + const safesOnChain1 = [ + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + ]; + const safesOnChain2 = [ + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + ]; + + networkService.get.mockImplementation(({ url }) => { + switch (url) { + case `${safeConfigUrl}/api/v1/chains`: { + return Promise.resolve({ + data: rawify( + pageBuilder() + .with('results', [chain1, chain2]) + .with('next', null) + .build(), + ), + status: 200, + }); + } + + case `${safeConfigUrl}/api/v1/chains/${chainId1}`: { + return Promise.resolve({ + data: rawify(chain1), + status: 200, + }); + } + + case `${safeConfigUrl}/api/v1/chains/${chainId2}`: { + return Promise.resolve({ + data: rawify(chain2), + status: 200, + }); + } + + // ValidationPipe checksums ownerAddress param + case `${chain1.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/`: { + return Promise.resolve({ + data: rawify({ safes: safesOnChain1 }), + status: 200, + }); + } + + case `${chain2.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/`: { + return Promise.resolve({ + data: rawify({ safes: safesOnChain2 }), + status: 200, + }); + } + + default: { + return Promise.reject(`No matching rule for url: ${url}`); + } + } + }); + + await request(app.getHttpServer()) + .get(`/v2/owners/${ownerAddress}/safes`) + .expect(200) + .expect({ + // Validation schema checksums addresses + [chainId1]: safesOnChain1.map((safe) => getAddress(safe)), + [chainId2]: safesOnChain2.map((safe) => getAddress(safe)), + }); + }); + + it('Success for multiple chain pages', async () => { + const ownerAddress = faker.finance.ethereumAddress(); + + const chainId1 = faker.string.numeric(); + const chainId2 = faker.string.numeric({ exclude: [chainId1] }); + + const chain1 = chainBuilder().with('chainId', chainId1).build(); + const chain2 = chainBuilder().with('chainId', chainId2).build(); + + const chainsUrl = `${safeConfigUrl}/api/v1/chains`; + const offset = 1; + const chainsPage1 = pageBuilder() + .with('results', [chain1]) + .with('next', limitAndOffsetUrlFactory(undefined, offset, chainsUrl)) + .build(); + const chainsPage2 = pageBuilder() + .with('results', [chain2]) + .with('next', null) + .build(); + + const safesOnChain1 = [ + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + ]; + const safesOnChain2 = [ + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + ]; + + networkService.get.mockImplementation(({ url, networkRequest }) => { + if (url === chainsUrl && !networkRequest!.params!.offset) { + return Promise.resolve({ + data: rawify(chainsPage1), + status: 200, + }); + } + if (url === chainsUrl && networkRequest!.params!.offset === offset) { + return Promise.resolve({ + data: rawify(chainsPage2), + status: 200, + }); + } + if (url === `${safeConfigUrl}/api/v1/chains/${chainId1}`) { + return Promise.resolve({ + data: rawify(chain1), + status: 200, + }); + } + + if (url === `${safeConfigUrl}/api/v1/chains/${chainId2}`) { + return Promise.resolve({ + data: rawify(chain2), + status: 200, + }); + } + + // ValidationPipe checksums ownerAddress param + if ( + url === + `${chain1.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/` + ) { + return Promise.resolve({ + data: rawify({ safes: safesOnChain1 }), + status: 200, + }); + } + + if ( + url === + `${chain2.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/` + ) { + return Promise.resolve({ + data: rawify({ safes: safesOnChain2 }), + status: 200, + }); + } + + return Promise.reject(`No matching rule for url: ${url}`); + }); + + await request(app.getHttpServer()) + .get(`/v2/owners/${ownerAddress}/safes`) + .expect(200) + .expect({ + // Validation schema checksums addresses + [chainId1]: safesOnChain1.map((safe) => getAddress(safe)), + [chainId2]: safesOnChain2.map((safe) => getAddress(safe)), + }); + }); + + it('should gracefully handle chain-specific Transaction Service error', async () => { + jest.spyOn(loggingService, 'warn'); + const ownerAddress = faker.finance.ethereumAddress(); + const chainId1 = faker.string.numeric(); + const chainId2 = faker.string.numeric({ exclude: [chainId1] }); + const chain1 = chainBuilder().with('chainId', chainId1).build(); + const chain2 = chainBuilder().with('chainId', chainId2).build(); + const safesOnChain1 = [ + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + faker.finance.ethereumAddress(), + ]; + networkService.get.mockImplementation(({ url }) => { + switch (url) { + case `${safeConfigUrl}/api/v1/chains`: { + return Promise.resolve({ + data: rawify( + pageBuilder() + .with('results', [chain1, chain2]) + .with('next', null) + .build(), + ), + status: 200, + }); + } + case `${safeConfigUrl}/api/v1/chains/${chainId1}`: { + return Promise.resolve({ + data: rawify(chain1), + status: 200, + }); + } + case `${safeConfigUrl}/api/v1/chains/${chainId2}`: { + return Promise.resolve({ + data: rawify(chain2), + status: 200, + }); + } + // ValidationPipe checksums ownerAddress param + case `${chain1.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/`: { + return Promise.resolve({ + data: rawify({ safes: safesOnChain1 }), + status: 200, + }); + } + case `${chain2.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/`: { + return Promise.reject(new Error('Test error')); + } + default: { + return Promise.reject(`No matching rule for url: ${url}`); + } + } + }); + + await request(app.getHttpServer()) + .get(`/v2/owners/${ownerAddress}/safes`) + .expect(200) + .expect({ + [chainId1]: safesOnChain1.map((safe) => getAddress(safe)), + [chainId2]: null, + }); + expect(loggingService.warn).toHaveBeenCalledTimes(1); + expect(loggingService.warn).toHaveBeenNthCalledWith( + 1, + `Failed to fetch Safe owners. chainId=${chainId2}`, + ); + }); + + it('Failure: Config API fails', async () => { + const ownerAddress = faker.finance.ethereumAddress(); + + networkService.get.mockImplementation(({ url }) => { + if (url === `${safeConfigUrl}/api/v1/chains`) { + const error = new NetworkResponseError( + new URL(`${safeConfigUrl}/api/v1/chains`), + { + status: 500, + } as Response, + ); + return Promise.reject(error); + } + return Promise.reject(`No matching rule for url: ${url}`); + }); + + await request(app.getHttpServer()) + .get(`/v2/owners/${ownerAddress}/safes`) + .expect(500) + .expect({ + message: 'An error occurred', + code: 500, + }); + + expect(networkService.get).toHaveBeenCalledTimes(1); + expect(networkService.get).toHaveBeenCalledWith({ + url: `${safeConfigUrl}/api/v1/chains`, + networkRequest: { + params: { limit: ChainsRepository.MAX_LIMIT, offset: 0 }, + }, + }); + }); + + it('Failure: data validation fails', async () => { + const ownerAddress = faker.finance.ethereumAddress(); + + const chainId = faker.string.numeric(); + + const chain = chainBuilder().with('chainId', chainId).build(); + + const safesOnChain = [ + faker.finance.ethereumAddress(), + faker.number.int(), + faker.finance.ethereumAddress(), + ]; + + networkService.get.mockImplementation(({ url }) => { + switch (url) { + case `${safeConfigUrl}/api/v1/chains`: { + return Promise.resolve({ + data: rawify({ + results: [chain], + }), + status: 200, + }); + } + + case `${safeConfigUrl}/api/v1/chains/${chainId}`: { + return Promise.resolve({ + data: rawify(chain), + status: 200, + }); + } + + case `${chain.transactionService}/api/v1/owners/${ownerAddress}/safes/`: { + return Promise.resolve({ + data: rawify({ safes: safesOnChain }), + status: 200, + }); + } + + default: { + return Promise.reject(`No matching rule for url: ${url}`); + } + } + }); + + await request(app.getHttpServer()) + .get(`/v2/owners/${ownerAddress}/safes`) + .expect(502) + .expect({ statusCode: 502, message: 'Bad gateway' }); + }); + }); +}); diff --git a/src/routes/owners/owners.controller.v2.ts b/src/routes/owners/owners.controller.v2.ts new file mode 100644 index 0000000000..c930c81439 --- /dev/null +++ b/src/routes/owners/owners.controller.v2.ts @@ -0,0 +1,31 @@ +import { Controller, Get, Param } from '@nestjs/common'; +import { ApiOkResponse, ApiTags } from '@nestjs/swagger'; +import { OwnersService } from '@/routes/owners/owners.service'; +import { ValidationPipe } from '@/validation/pipes/validation.pipe'; +import { AddressSchema } from '@/validation/entities/schemas/address.schema'; + +@ApiTags('owners') +@Controller({ + path: '', + version: '2', +}) +export class OwnersControllerV2 { + constructor(private readonly ownersService: OwnersService) {} + + @ApiOkResponse({ + schema: { + type: 'object', + additionalProperties: { + type: 'array', + items: { type: 'string' }, + }, + }, + }) + @Get('owners/:ownerAddress/safes') + async getAllSafesByOwner( + @Param('ownerAddress', new ValidationPipe(AddressSchema)) + ownerAddress: `0x${string}`, + ): Promise<{ [chainId: string]: Array | null }> { + return this.ownersService.getAllSafesByOwner({ ownerAddress }); + } +} diff --git a/src/routes/owners/owners.module.ts b/src/routes/owners/owners.module.ts index 7dcbe66bb5..81841447f4 100644 --- a/src/routes/owners/owners.module.ts +++ b/src/routes/owners/owners.module.ts @@ -1,11 +1,12 @@ import { Module } from '@nestjs/common'; -import { OwnersController } from '@/routes/owners/owners.controller'; +import { OwnersControllerV1 } from '@/routes/owners/owners.controller.v1'; +import { OwnersControllerV2 } from '@/routes/owners/owners.controller.v2'; import { OwnersService } from '@/routes/owners/owners.service'; import { SafeRepositoryModule } from '@/domain/safe/safe.repository.interface'; @Module({ imports: [SafeRepositoryModule], - controllers: [OwnersController], + controllers: [OwnersControllerV1, OwnersControllerV2], providers: [OwnersService], }) export class OwnersModule {} diff --git a/src/routes/owners/owners.service.ts b/src/routes/owners/owners.service.ts index 9fa12c60a7..1ef72ee1d0 100644 --- a/src/routes/owners/owners.service.ts +++ b/src/routes/owners/owners.service.ts @@ -17,6 +17,8 @@ export class OwnersService { return this.safeRepository.getSafesByOwner(args); } + // TODO: Remove with /owners/:ownerAddress/safes + // @deprecated async deprecated__getAllSafesByOwner(args: { ownerAddress: `0x${string}`; }): Promise<{ [chainId: string]: Array }> {