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

Subgraph Composition : Fix graph init for composed subgraphs #1920

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

incrypto32
Copy link
Member

@incrypto32 incrypto32 commented Jan 27, 2025

This PR enables graph init for composed subgraphs. It creates a new ipfs client to validate the source subgraph manifest

Copy link

changeset-bot bot commented Jan 27, 2025

🦋 Changeset detected

Latest commit: 3a75a7f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@graphprotocol/graph-cli Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Jan 27, 2025

🚀 Snapshot Release (alpha)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package Version Info
@graphprotocol/graph-cli 0.96.0-alpha-20250130081352-3a75a7fd2231fb7cd2ed22c8020b1dd5539efa70 npm ↗︎ unpkg ↗︎

Copy link

cloudflare-workers-and-pages bot commented Jan 27, 2025

Deploying graph-tooling with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3a75a7f
Status: ✅  Deploy successful!
Preview URL: https://384fc2da.graph-tooling.pages.dev
Branch Preview URL: https://krishna-comp-fixes.graph-tooling.pages.dev

View logs

@YaroShkvorets
Copy link
Collaborator

@incrypto32 could you fix the linter error?

@YaroShkvorets
Copy link
Collaborator

I think there are some issues with running github actions in your branch after #1919

Rebasing on top of the latest main should help.

@incrypto32
Copy link
Member Author

@YaroShkvorets i just rebase on top of main and the issue persists. I cant figure out what the lint issue is, also im not able to run the lint locally. Getting these error


Oops! Something went wrong! :(

ESLint: 9.19.0

Error: Cannot read config file: /Users/krishna/dev/work/edgeandnode/thegraph/graph-tooling/node_modules/.pnpm/@[email protected][email protected][email protected][email protected]/node_modules/@theguild/eslint-config/src/index.js
Error: Failed to patch ESLint because the calling module was not recognized.
If you are using a newer ESLint version that may be unsupported, please create a GitHub issue:
https://github.com/microsoft/rushstack/issues
Referenced from: 
    at Object.<anonymous> (/Users/krishna/dev/work/edgeandnode/thegraph/graph-tooling/node_modules/.pnpm/@[email protected]/node_modules/@rushstack/eslint-patch/lib/_patch-base.js:167:19)
    at Module._compile (node:internal/modules/cjs/loader:1469:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
    at Module.load (node:internal/modules/cjs/loader:1288:32)
    at Module._load (node:internal/modules/cjs/loader:1104:12)
    at Module.require (node:internal/modules/cjs/loader:1311:19)
    at require (node:internal/modules/helpers:179:18)
    at Object.<anonymous> (/Users/krishna/dev/work/edgeandnode/thegraph/graph-tooling/node_modules/.pnpm/@[email protected]/node_modules/@rushstack/eslint-patch/lib/modern-module-resolution.js:11:23)
    at Module._compile (node:internal/modules/cjs/loader:1469:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
    
    ```

@YaroShkvorets
Copy link
Collaborator

Looks like it's passing. Good for review now?

@incrypto32
Copy link
Member Author

@YaroShkvorets yes, ready for review

Copy link
Collaborator

@YaroShkvorets YaroShkvorets left a comment

Choose a reason for hiding this comment

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

  • Remove hidden: true from --ipfs flag on line 117
  • There doesn't seem to be a way to scaffold a subgraph-based subgraph in non-interactive way from command line. Is it possible?
  • this command does nothing and exits with code 0 without any error: graph init test test --from-contract=0x0 --protocol=subgraph --network=mainnet

initial: ipfsUrl,
skip: () => !isComposedSubgraph,
result: value => {
ipfsNode = value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's check that this is at least a valid URL

if (isComposedSubgraph) {
return value.startsWith('Qm') ? true : 'Subgraph deployment ID must start with Qm';
if (!ipfsNode) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't allow empty ipfsNode. Return corresponding error message.
Also, if we validate ipfsNode above this would be unnecessary.

// Get network from first data source
const sourceNetwork = allSources[0].network;
if (!sourceNetwork) {
return { valid: true }; // Network not specified in source, skip validation
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible?
And if possible, why is it valid?

}

const normalizedSourceNetwork = sourceNetwork.toLowerCase();
const normalizedTargetNetwork = targetNetwork.toLowerCase();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to normalize here. If somehow our targetNetwork is Mainnet it shouldn't let us continue. Because in graph-node network ID is case-sensitive.

@@ -822,6 +834,22 @@ async function processInitForm(

await promptManager.executeInteractive();

// Validate network matches if loading from IPFS
if (ipfsNode && source?.startsWith('Qm')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not check for isComposedSubgraph instead?

I think we can move this validation into initSubgraphFromContract(). We are already creating ipfsClient anyway and doing some additional validations there. This method is to process init form.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

3 participants