indexer-common,indexer-agent: auto graft resolve with limit #481

hopeyen · 2022-09-12T16:52:57Z

Currently, to index a subgraph deployment with grafting dependencies, indexers must manually deploy and index the graft base deployments.

To enable auto resolve feature on the indexer-agent, we add new start-up parameter

auto-graft-resolver-limit the maximum depth of grafting dependency to automatically resolve (number, default is 0)
ipfs-endpoint the endpoint to an ipfs node to query subgraph manifest data for graft specifications before deploying a subgraph

In particular, these two parameters, along with indexingStatusResolver, are passed into SubgraphManger

  statusResolver: IndexingStatusResolver
  autoGraftResolverLimit: number                 // default 0 such that the agent will not automatically resolve any grafting dependency. 
  ipfsEndpoint?: string

These 2 parameters allow indexers to control the depth of grafting dependency the agent should auto resolve. If the dependency depth is within the provided limit (inclusive), the agent start by indexing the root of the dependencies.

If the reconciliation conflict comes from indexingRules, then agent attempts the target deployment periodically.
If allocating with actionQueue while the grafting dependencies aren't resolved, then indexer agent will automatically deploy the base level deployment and fails the action.
indexingStatusResolver is used to check for grafting deployment's status before trying to ensure the indexing status of the target deployment.

For testing purposes I made these random subgraphs with dependencies on the testnet

indexing goerli, graft depth of 2, QmU21JhpGJmLimvFwWWdFiGftf8BRbqGdSxwHhCatWDC2m
indexing mainnet, graft depth of 2, QmSxFyuizxc42A8gAXXzSu34wndzWxswEQUURqhghms4J7

Resolves #439

tilacog

@hopeyen Thanks for this!

I'm adding these review comments as notes for when I rebase this PR due to the recent L2 changes.

tilacog · 2023-10-10T17:28:08Z

packages/indexer-agent/src/__tests__/indexer.ts

@@ -126,6 +126,7 @@ const setup = async () => {
  await sequelize.sync({ force: true })

  const statusEndpoint = 'http://localhost:8030/graphql'
+  const ipfsEndpoint = 'https://ipfs.network.thegraph.com'


This should be configurable with the current value as the default.

tilacog · 2023-10-10T17:29:19Z

packages/indexer-agent/src/__tests__/indexer.ts

@@ -139,6 +140,7 @@ const setup = async () => {
  })

  const indexNodeIDs = ['node_1']
+  const autoGraftResolverLimit = 1


Consider setting this value to zero to disable the feature in tests, except when testing the feature itself.

tilacog · 2023-10-10T17:33:01Z

packages/indexer-agent/src/agent.ts

+        try {
+          this.indexer.ensure(name, deployment)
+        } catch {
+          this.indexer.resolveGrafting(name, deployment, 0)
+        }


Let's ensure we accurately identify and handle all potential errors here, considering that resolving grafts may introduce its own errors.

tilacog · 2023-10-10T17:34:20Z

packages/indexer-agent/src/commands/start.ts

+        if (
+          !Number.isInteger(argv['auto-graft-resolver-limit']) ||
+          argv['auto-graft-resolver-limit'] < 0
+        ) {
+          return 'Invalid --auto-graft-resolver-limit provided. Must be >= 0 and an integer.'
+        }


We could rely on yargs' coercion instead of manually implementing this validation.
https://yargs.js.org/docs/#api-reference-coercekey-fn

tilacog · 2023-10-10T17:42:35Z

packages/indexer-agent/src/indexer.ts

  ) {
    this.indexerManagement = indexerManagement
    this.statusResolver = statusResolver
    this.logger = logger
    this.indexerAddress = indexerAddress
    this.allocationManagementMode = allocationManagementMode
+    this.autoGraftResolverLimit = autoGraftResolverLimit
+    this.ipfsEndpoint = ipfsUrl + '/api/v0/cat?arg='


I prefer using the URL type when constructing URLs like this.
We get the benefit of it being checked for correctness in runtime.

tilacog · 2023-10-10T20:51:14Z

packages/indexer-common/src/indexer-management/subgraphs.ts

+  // Simple fetch for subgraph manifest
+  async subgraphManifest(targetDeployment: SubgraphDeploymentID) {
+    const ipfsFile = await fetch(this.ipfsEndpoint + targetDeployment.ipfsHash, {
+      method: 'POST',
+      redirect: 'follow',
+    })
+    return yaml.parse(await ipfsFile.text())
+  }


I'd consider handling IO and throwing errors with more meaningful messages here.

tilacog · 2023-10-10T20:51:44Z

packages/indexer-common/src/indexer-management/resolvers/allocations.ts

+        models,
+        subgraphDeploymentID,
+        indexNode,
+        0,


Why do we set grafting depth to zero here?

IIRC the depth is set to 0 as the initialization to the recursive resolveGrafting which should stop when depth gets incremented to the depths limit

tilacog · 2023-10-10T22:00:42Z

packages/indexer-common/src/indexer-management/subgraphs.ts

+        graftStatus[0].chains[0].latestBlock &&
+        graftStatus[0].chains[0].latestBlock.number >= manifest.graft.block


I'd validate the array length before accessing elements by index.

tilacog · 2023-10-10T22:02:45Z

packages/indexer-common/src/indexer-management/subgraphs.ts

+    } catch {
+      throw indexerError(
+        IndexerErrorCode.IE074,
+        `Base deployment hasn't synced to the graft block, try again later`,
+      )
+    }


I don't get why the error message here is the same as the one right above.

Also, we might be interested in actually catching the original error and propagating it within the IndexerError.

tilacog · 2023-10-10T22:03:45Z

packages/indexer-common/src/indexer-management/subgraphs.ts

+  async stop_sync(
+    logger: Logger,
+    deployment: SubgraphDeploymentID,
+    models: IndexerManagementModels,
+  ) {
+    const neverIndexingRule = {
+      identifier: deployment.ipfsHash,
+      identifierType: SubgraphIdentifierType.DEPLOYMENT,
+      decisionBasis: IndexingDecisionBasis.NEVER,
+    } as Partial<IndexingRuleAttributes>
+
+    await upsertIndexingRule(logger, models, neverIndexingRule)
+  }


I'm unsure if a similar function is already defined elsewhere.

hopeyen force-pushed the hope/grafting-auto-resolve branch from 68dd05a to d74033f Compare September 12, 2022 17:10

hopeyen force-pushed the hope/grafting-auto-resolve branch 2 times, most recently from 0608a51 to edb1d02 Compare October 5, 2022 23:32

hopeyen marked this pull request as ready for review October 5, 2022 23:32

hopeyen force-pushed the hope/grafting-auto-resolve branch from edb1d02 to 87ba4f0 Compare March 31, 2023 13:24

indexer-common,indexer-agent: auto graft resolve

2f9759a

hopeyen force-pushed the hope/grafting-auto-resolve branch from 87ba4f0 to 2f9759a Compare March 31, 2023 13:28

hopeyen requested review from tilacog and fordN March 31, 2023 21:14

tilacog reviewed Oct 10, 2023

View reviewed changes

tilacog mentioned this pull request Nov 9, 2023

Conform grafting auto-resolve feature to latest main #805

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

indexer-common,indexer-agent: auto graft resolve with limit #481

indexer-common,indexer-agent: auto graft resolve with limit #481

hopeyen commented Sep 12, 2022 •

edited

Loading

tilacog left a comment

tilacog Oct 10, 2023

tilacog Oct 10, 2023

tilacog Oct 10, 2023

tilacog Oct 10, 2023

tilacog Oct 10, 2023

tilacog Oct 10, 2023

tilacog Oct 10, 2023

hopeyen Oct 14, 2023

tilacog Oct 10, 2023

tilacog Oct 10, 2023

tilacog Oct 10, 2023

		graftStatus[0].chains[0].latestBlock &&
		graftStatus[0].chains[0].latestBlock.number >= manifest.graft.block

indexer-common,indexer-agent: auto graft resolve with limit #481

Are you sure you want to change the base?

indexer-common,indexer-agent: auto graft resolve with limit #481

Conversation

hopeyen commented Sep 12, 2022 • edited Loading

tilacog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hopeyen commented Sep 12, 2022 •

edited

Loading