Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Iterate all chains when fetching all Safes #2002

Merged
merged 7 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/domain/chains/chains.repository.interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ export interface IChainsRepository {
*/
getChains(limit?: number, offset?: number): Promise<Page<Chain>>;

/**
* Gets all the {@link Chain} available across pages
*/
getAllChains(): Promise<Array<Chain>>;

/**
* Gets the {@link Chain} associated with {@link chainId}
*
Expand Down
22 changes: 22 additions & 0 deletions src/domain/chains/chains.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
} from '@/domain/indexing/entities/indexing-status.entity';
import { ILoggingService, LoggingService } from '@/logging/logging.interface';
import { differenceBy } from 'lodash';
import { PaginationData } from '@/routes/common/pagination/pagination.data';

@Injectable()
export class ChainsRepository implements IChainsRepository {
Expand Down Expand Up @@ -47,6 +48,27 @@ export class ChainsRepository implements IChainsRepository {
return valid;
}

async getAllChains(): Promise<Array<Chain>> {
PooyaRaki marked this conversation as resolved.
Show resolved Hide resolved
const chains: Array<Chain> = [];

let offset: number | undefined;

while (true) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be a bit more defensive here: while(true) seems scary, and we are relying on the fact the datasource (Config Service in this case) works as expected and does not have a bug affecting the offset value.

What do you think about having some kind of threshold to limit the max number of iterations in the loop? We could define a max number of Chain objects that can be handled in total and divide it by the default limit path param.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've set an "upper request limit" (adjustable by env. var.) so that only n amount of subsequent requests are made. We also now fetch the upper limit to hopefully also prevent subsequent requests. As of 6b4f5f7.

const result = await this.getChains(undefined, offset);
chains.push(...result.results);

if (!result.next) {
break;
}

const url = new URL(result.next);
const paginationData = PaginationData.fromLimitAndOffset(url);
offset = paginationData.offset;
}

return chains;
}

async getSingletons(chainId: string): Promise<Singleton[]> {
const transactionApi = await this.transactionApiManager.getApi(chainId);
const singletons = await transactionApi.getSingletons();
Expand Down
6 changes: 2 additions & 4 deletions src/domain/safe/safe.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -428,11 +428,9 @@ export class SafeRepository implements ISafeRepository {
async getAllSafesByOwner(args: {
ownerAddress: `0x${string}`;
}): Promise<{ [chainId: string]: Array<string> }> {
// Note: does not take pagination into account but we do not support
// enough chains for it to be an issue
const { results } = await this.chainsRepository.getChains();
const chains = await this.chainsRepository.getAllChains();
const allSafeLists = await Promise.all(
results.map(async ({ chainId }) => {
chains.map(async ({ chainId }) => {
const safeList = await this.getSafesByOwner({
chainId,
ownerAddress: args.ownerAddress,
Expand Down
104 changes: 101 additions & 3 deletions src/routes/owners/owners.controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ 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 { pageBuilder } from '@/domain/entities/__tests__/page.builder';
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';
Expand Down Expand Up @@ -185,7 +188,7 @@ describe('Owners Controller (Unit)', () => {
});

describe('GET all safes by owner address', () => {
it('Success', async () => {
it('Success for singular chain page', async () => {
const ownerAddress = faker.finance.ethereumAddress();

const chainId1 = faker.string.numeric();
Expand All @@ -209,7 +212,10 @@ describe('Owners Controller (Unit)', () => {
switch (url) {
case `${safeConfigUrl}/api/v1/chains`: {
return Promise.resolve({
data: pageBuilder().with('results', [chain1, chain2]).build(),
data: pageBuilder()
.with('results', [chain1, chain2])
.with('next', null)
.build(),
status: 200,
});
}
Expand Down Expand Up @@ -259,6 +265,98 @@ describe('Owners Controller (Unit)', () => {
});
});

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: chainsPage1,
status: 200,
});
}
if (url === chainsUrl && networkRequest!.params!.offset === offset) {
return Promise.resolve({
data: chainsPage2,
status: 200,
});
}
if (url === `${safeConfigUrl}/api/v1/chains/${chainId1}`) {
return Promise.resolve({
data: chain1,
status: 200,
});
}

if (url === `${safeConfigUrl}/api/v1/chains/${chainId2}`) {
return Promise.resolve({
data: chain2,
status: 200,
});
}

// ValidationPipe checksums ownerAddress param
if (
url ===
`${chain1.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/`
) {
return Promise.resolve({
data: { safes: safesOnChain1 },
status: 200,
});
}

if (
url ===
`${chain2.transactionService}/api/v1/owners/${getAddress(ownerAddress)}/safes/`
) {
return Promise.resolve({
data: { safes: safesOnChain2 },
status: 200,
});
}

return Promise.reject(`No matching rule for url: ${url}`);
});

await request(app.getHttpServer())
.get(`/v1/owners/${ownerAddress}/safes`)
.expect(200)
.expect({
// Validation schema checksums addresses
[chainId1]: safesOnChain1.map((safe) => getAddress(safe)),
[chainId2]: safesOnChain2.map((safe) => getAddress(safe)),
});
});

it('Failure: Config API fails', async () => {
const ownerAddress = faker.finance.ethereumAddress();

Expand Down