Iterate all chains when fetching all Safes #2002
Conversation
| const url = new URL(result.next); | ||
| const paginationData = PaginationData.fromLimitAndOffset(url); | ||
| offset ??= 0; | ||
| offset += paginationData.offset; |
There was a problem hiding this comment.
I think this is not correct. Considering for example 10 items per page (limit), it would work for the second page (offset was 0, + 10 = 10) and also for the third (previous offset is 10, + 10 = 20), but for the fourth it would take 20 as previous offset and the next URL would contain ?offset=30, right? So it would set 50 as new offset.
Maybe my math is wrong but I think we need to set the offset directly to the one extracted from the URL.
offset = paginationData.offset;There was a problem hiding this comment.
Good point. The offset is indeed "fixed". I change it in 16fcf76.
|
|
||
| let offset: number | undefined; | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| @Injectable() | ||
| export class ChainsRepository implements IChainsRepository { | ||
| private readonly maxLimit: number; |
There was a problem hiding this comment.
Nit: Why is this named max? I mean, it seems the default limit we use for all the requests to get the chains. Maybe I'm wrong but I'd expect this to be something like a threshold but it is like a constant. Is there any reason to not use PaginationData.DEFAULT_LIMIT for this?
There was a problem hiding this comment.
It mirrors the max_limit of the Chains pagination from the Config. Service.
I did consider changing the PaginationData.DEFAULT_LIMIT instead but that affects all pagination requests across the project. If we did, it would reduce the request by the Web though as they don't provide a limit in their request.
What do you think?
There was a problem hiding this comment.
I meant doing something like:
const result = await this.getChains(PaginationData.DEFAULT_LIMIT, offset);I didn't mean to change the PaginationData.DEFAULT_LIMIT, that would indeed affect the overall service behavior. Sorry about the lack of clarity in my previous comment 😄
There was a problem hiding this comment.
The use of 40 was at the request of @PooyaRaki.
@PooyaRaki, what do you think about this?
There was a problem hiding this comment.
Changing the default limit could be risky, as it would impact all endpoints. However, since the web isn't specifying any limits, for this particular case, I think it's better to set a maximum limit of for example 40 specifically for this endpoint.
There was a problem hiding this comment.
Considering the above, I've moved the limit to a constant within the repository as I don't see it changing (for now). I left the sequential page requests in the configuration though.
What are your thoughts?
Summary
Resolves #2001
When getting all Safes by owner address, we fetch the chain configurations. We don't, however, take into account pagination.
This adds a new
IChainsRepository['getAllChains']method that fetches subsequentnextpages and concatenatesresults, called in thegetAllSafesByOwnermethod.Changes
IChainsRepository['getAllChains']implementationgetAllChainsinstead ofgetChainsingetAllSafesByOwner