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

Integrate parsing and entity logic for contracts and events #182

Open
wants to merge 5 commits into
base: mainnet-staging
Choose a base branch
from

Conversation

mnlesane
Copy link
Contributor

schema.graphql Outdated
@@ -522,6 +522,9 @@ type SubgraphDeployment @entity {
activeSubgraphCount: Int!
"Amount of Subgraph entities that were currently using this deployment when they got deprecated"
deprecatedSubgraphCount: Int!

"The contract sources in this subgraph deployment's manifest"
contracts: [Contract!]!
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would probably modify this to use a many-to-many relationship auxiliary entity like described in the docs.
It's unfortunately a little bit extra effort for querying, but a lot better for indexing performance (mostly db size).



//Associate Contracts and Events with Deployment
processManifestForContracts(subgraph,deployment)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious, is given that we have functions to abstract the notion of getting new metadata from IPFS, what was the reason to directly call this function in the specific event handlers directly, instead of calling it in the function that fetches the metadata? (I'd assume it's a more direct integration, but we would be calling ipfs.cat twice, although not sure how much would that affect performance, and I also think it might be because you wanted to avoid having to change the interface of those fetch functions)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm referring to fetchSubgraphDeploymentManifest, which would already fetch the manifest and store it as a String representation of the file in the SubgraphDeployment entity btw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a bit of a design relic from the original subgraph, but seeing as the manifest is only retrieved once per SubgraphDeployment entity, I replaced the IPFS retrieval with processing SubgraphDeployment.manifest and moved the call to createOrLoadSubgraphDeployment. Let me know if i I should move the call into fetchSubgraphDeploymentManifest instead.

@juanmardefago
Copy link
Collaborator

Another thing I forgot to mention (and this one is on me) is that in our workflow we first merge to the branch mainnet-staging and then move it to master once it's synced and tested in staging environments.

I should probably update the README.md as well as CONTRIBUTING.md files, since there's probably no mention of this anywhere

@mnlesane mnlesane marked this pull request as draft April 26, 2022 02:25
@mnlesane mnlesane changed the base branch from master to mainnet-staging May 3, 2022 18:11
@mnlesane mnlesane marked this pull request as ready for review May 3, 2022 18:38
@juanmardefago
Copy link
Collaborator

@mnlesane sorry for the long delay, I've been caught in a ton of stuff and missed these updates.

Overall it looks a lot cleaner now and I really like the approach!

Only thing missing is to double check that it's working as expected (I'd love to have tests in the subgraph soon, but for now at least double checking it on a synced version would be just fine)

@mnlesane
Copy link
Contributor Author

mnlesane commented Aug 6, 2022

@juanmardefago All changes have been verified via a fork that was deployed to the hosted service. I wrote a simple test a while back for standardizeAddresses(), which I've added assertions to and checked into this branch. If unit tests should also be created for the parsing logic, please let me know.

@juanmardefago
Copy link
Collaborator

juanmardefago commented Oct 24, 2022

@mnlesane I mentioned I was going to merge this on #192, but it seems that the link you sent has a failed subgraph there. Just double checking, is the failed subgraph running the same code as this branch?

Happy to merge this if it's not, otherwise it'd be good to double check what happened before merging it, so we can fix it beforehand!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants