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

Reverse 1:n relationships #37

Merged
merged 6 commits into from
May 19, 2020
Merged

Reverse 1:n relationships #37

merged 6 commits into from
May 19, 2020

Conversation

lutter
Copy link
Collaborator

@lutter lutter commented May 15, 2020

This pull request reverse how 1:n associations are stored in Uniswap: rather than storing them as an array on the 'n' side, they are now stored as a scalar attribute on the '1' side. Doing it this way makes things much easier for the database when executing queries.

One downside is that changing which side is stored and which side is derived changes what filters are available - if that conflicts with how Uniswap needs to be queried, feel free to ignore any of this.

I did sync with this version of Uniswap on a test system successfully, but I don't know the ins and outs of the subgraph enough to be able to tell whether I broke some other functionality.

Copy link
Contributor

@davekaj davekaj left a comment

Choose a reason for hiding this comment

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

The code looks good (other than adding the !). We need to ask @ianlapham if this change would effect their front end because of the filter changes.

Also, maybe we should announce it in discord? Maybe others use this subgraph?

@@ -60,7 +60,7 @@ type Exchange @entity {
weightedAvgPriceUSD: BigDecimal! # weightAvgPriceUSD = ( $1 of ETH in ETH ) / weightedAvgPrice

# Fields used to help derived relationship
factory: Uniswap! @derivedFrom(field: "exchanges")
factory: Uniswap
Copy link
Contributor

Choose a reason for hiding this comment

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

We can add a ! to this

@davekaj
Copy link
Contributor

davekaj commented May 15, 2020

Merging this would close #28

@ianlapham
Copy link
Contributor

This makes sense to me - if the performance would greatly improve it might be worth it to update our queries on our frotn-end to support the changes. Is there a way to deploy this to some other subgraph so I could test it first?

@davekaj
Copy link
Contributor

davekaj commented May 15, 2020

I just deployed it, but I assume lutter already deployed it somewhere because it was instantly synced for me. You can see it here https://thegraph.com/explorer/subgraph/davekaj/uniswap

@lutter
Copy link
Collaborator Author

lutter commented May 16, 2020

Yes, I had deployed it a while ago, and completely forgotten about it; it's also at https://thegraph.com/explorer/subgraph/lutter/uniswap-rels

@ianlapham
Copy link
Contributor

I just tested this with our v1 info client - it actually seems to work with no changes required to our queries. If this helps us sync faster through grafting I'm ok to merge this in. @davekaj @lutter

@davekaj
Copy link
Contributor

davekaj commented May 19, 2020

great, will merge now

@davekaj davekaj merged commit 946bd10 into master May 19, 2020
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