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

indexer-common: Avoid using createdAt field for pagination #914

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

fordN
Copy link
Contributor

@fordN fordN commented Jun 17, 2024

Changes

  • Update several NetworkMonitor functions to paginate by id instead of by createdAt. Functions updated: subgraphs(), subgraphDeployments() and disputableAllocations().

Background

On L2 networks, like arbitrum-one, createdAt is not necessarily unique, so paginating based on createdAt can lead to missing items.

@fordN fordN self-assigned this Jun 17, 2024
@fordN fordN added the bug Something isn't working label Jun 17, 2024
@fordN fordN requested a review from aasseman June 17, 2024 22:10
Copy link
Contributor

@dwerner dwerner left a comment

Choose a reason for hiding this comment

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

lgtm! thanks

Copy link
Contributor

@aasseman aasseman left a comment

Choose a reason for hiding this comment

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

Just nitpicking on adhering to the subgraph's schema, though graph-node may currently allow these things.

Also, somewhat unrelated, I found a few places in the code where pagination isn't implemented at all. In particular that could cause issues if somehow an indexer managed to open more than 1000 allocations. We know that some indexers are already indexing more than 1000 subgraphs at a time, so it's not completely impossible.

@aasseman aasseman linked an issue Jun 19, 2024 that may be closed by this pull request
@dwerner dwerner self-requested a review June 24, 2024 23:12
@dwerner
Copy link
Contributor

dwerner commented Jun 24, 2024

Removing my approval as I've made changes now.

Copy link
Contributor

@aasseman aasseman left a comment

Choose a reason for hiding this comment

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

One of the comments was not addressed. GitHub won't let me explicitly select the piece of code....

So here's the issue:
packages/indexer-common/src/indexer-management/monitor.ts:1139 $createdAt: Int! should be $lastId: ID!

@dwerner
Copy link
Contributor

dwerner commented Jun 25, 2024

One of the comments was not addressed. GitHub won't let me explicitly select the piece of code....

So here's the issue: packages/indexer-common/src/indexer-management/monitor.ts:1139 $createdAt: Int! should be $lastId: ID!

Thanks for catching this.

Copy link
Contributor

@dwerner dwerner left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@fordN fordN merged commit c6351ab into main Jun 28, 2024
10 checks passed
@fordN fordN deleted the ford/paginate-by-id branch June 28, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: 🚗 Merged
Development

Successfully merging this pull request may close these issues.

indexer-agent: missing subgraphs
3 participants