From b89345fe62cff96095322e65ccb867cec19b7b82 Mon Sep 17 00:00:00 2001 From: Aaron Cook Date: Wed, 22 Jan 2025 16:31:01 +0100 Subject: [PATCH] Add `/v2/owners/:ownerAddress/safes` to gracefully handle errors (#2266) Adds a new endpoint (`/v2/owners/:ownerAddress/safes`) which returns `null` for every chain which the Transaction Service threw for: - Add new scaffolding for v2 endpoint, deprecating previous. - Wrap Transaction Service requests in `Promise.allSettled`. - Construct all Safe lists from successful requests, logging for errorneous ones. - Add appropriate test coverage. --- src/domain/safe/safe.repository.interface.ts | 6 +- src/domain/safe/safe.repository.ts | 40 +- ...r.spec.ts => owners.controller.v1.spec.ts} | 0 ....controller.ts => owners.controller.v1.ts} | 4 +- .../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 | 10 +- 8 files changed, 495 insertions(+), 7 deletions(-) rename src/routes/owners/{owners.controller.spec.ts => owners.controller.v1.spec.ts} (100%) rename src/routes/owners/{owners.controller.ts => owners.controller.v1.ts} (90%) 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.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 31d81a0d0e..ee06417258 100644 --- a/src/domain/safe/safe.repository.ts +++ b/src/domain/safe/safe.repository.ts @@ -423,7 +423,9 @@ export class SafeRepository implements ISafeRepository { return SafeListSchema.parse(safeList); } - async getAllSafesByOwner(args: { + // TODO: Remove with /owners/:ownerAddress/safes + // @deprecated + async deprecated__getAllSafesByOwner(args: { ownerAddress: `0x${string}`; }): Promise<{ [chainId: string]: Array }> { const chains = await this.chainsRepository.getAllChains(); @@ -449,6 +451,42 @@ export class SafeRepository implements ISafeRepository { }, {}); } + 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 }) => { + const safeList = await this.getSafesByOwner({ + chainId, + ownerAddress: args.ownerAddress, + }); + + return { + chainId, + safeList, + }; + }), + ); + + const result: { [chainId: string]: Array | null } = {}; + + for (const [index, allSafeList] of allSafeLists.entries()) { + const chainId = chains[index].chainId; + + if (allSafeList.status === 'fulfilled') { + result[chainId] = allSafeList.value.safeList.safes; + } else { + result[chainId] = null; + this.loggingService.warn( + `Failed to fetch Safe owners. chainId=${chainId}`, + ); + } + } + + return result; + } + async getLastTransactionSortedByNonce(args: { chainId: string; safeAddress: `0x${string}`; diff --git a/src/routes/owners/owners.controller.spec.ts b/src/routes/owners/owners.controller.v1.spec.ts similarity index 100% rename from src/routes/owners/owners.controller.spec.ts rename to src/routes/owners/owners.controller.v1.spec.ts diff --git a/src/routes/owners/owners.controller.ts b/src/routes/owners/owners.controller.v1.ts similarity index 90% rename from src/routes/owners/owners.controller.ts rename to src/routes/owners/owners.controller.v1.ts index c57e89545f..1c9d8ac80e 100644 --- a/src/routes/owners/owners.controller.ts +++ b/src/routes/owners/owners.controller.v1.ts @@ -10,7 +10,7 @@ import { AddressSchema } from '@/validation/entities/schemas/address.schema'; path: '', version: '1', }) -export class OwnersController { +export class OwnersControllerV1 { constructor(private readonly ownersService: OwnersService) {} @ApiOkResponse({ type: SafeList }) @@ -29,6 +29,6 @@ export class OwnersController { @Param('ownerAddress', new ValidationPipe(AddressSchema)) ownerAddress: `0x${string}`, ): Promise<{ [chainId: string]: Array }> { - return this.ownersService.getAllSafesByOwner({ ownerAddress }); + return this.ownersService.deprecated__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 28b4fef921..1ef72ee1d0 100644 --- a/src/routes/owners/owners.service.ts +++ b/src/routes/owners/owners.service.ts @@ -17,9 +17,17 @@ export class OwnersService { return this.safeRepository.getSafesByOwner(args); } - async getAllSafesByOwner(args: { + // TODO: Remove with /owners/:ownerAddress/safes + // @deprecated + 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); } }