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

Feature/fetch task from gelato #104

Merged

Conversation

kassandraoftroy
Copy link
Contributor

@kassandraoftroy kassandraoftroy commented Dec 2, 2020

New feature for omen-subgraph to track tasks on Gelato.

EDIT: we had some issues compiling on different setups (dependency versioning issues) but we eventually resolved the issues and have upgraded from draft PR to "ready for review"

@kassandraoftroy kassandraoftroy marked this pull request as ready for review December 3, 2020 17:40
@kassandraoftroy kassandraoftroy force-pushed the feature/fetch-task-from-gelato branch 3 times, most recently from 1764112 to 09c5d2f Compare December 7, 2020 16:34
Copy link
Contributor

@cag cag left a comment

Choose a reason for hiding this comment

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

I'm kind of confused actually. This part of the subgraph looks completely self-contained. Couldn't it just exist as a separate Gelato subgraph? What's the value in bundling this code along with the Omen subgraph?

"@graphprotocol/graph-cli": "^0.18.0",
"@graphprotocol/graph-ts": "^0.18.1",
"@graphprotocol/graph-cli": "^0.19.0",
"@graphprotocol/graph-ts": "^0.19.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting, I made a conflict by merging something earlier, oops...

Should be an easy fix. I actually prefer the way it is written in this PR.

Copy link
Contributor

@hilmarx hilmarx Dec 17, 2020

Choose a reason for hiding this comment

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

fixed in d8c9763

package.json Outdated
@@ -63,7 +63,7 @@
"realitio-gnosis-proxy": "github:gnosis/realitio-gnosis-proxy",
"replace-in-file": "^6.1.0",
"should": "^13.2.3",
"truffle": "^5.1.18",
"truffle": "5.1.35",
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, should Truffle be version pinned?

Copy link
Contributor

@hilmarx hilmarx Dec 17, 2020

Choose a reason for hiding this comment

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

nope, fixed to ^5.1.35 in d8c9763

schema.graphql Outdated
@@ -261,6 +261,82 @@ type FpmmTransaction @entity {
transactionHash: Bytes!
}

type User @entity {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to GelatoUser?

Copy link
Contributor

@hilmarx hilmarx Dec 17, 2020

Choose a reason for hiding this comment

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

renamed to GelatoUser in d8c9763 to make things clearer.

kind: ethereum/events
apiVersion: 0.0.3
language: wasm/assemblyscript
entities:
Copy link
Contributor

Choose a reason for hiding this comment

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

Entities are the @entity definitions in the schema.graphql, not events or handlers.

Copy link
Contributor

@hilmarx hilmarx Dec 17, 2020

Choose a reason for hiding this comment

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

fixed in d8c9763

@hilmarx
Copy link
Contributor

hilmarx commented Dec 17, 2020

I'm kind of confused actually. This part of the subgraph looks completely self-contained. Couldn't it just exist as a separate Gelato subgraph? What's the value in bundling this code along with the Omen subgraph?

@cag so that is actually what we started with and we also have isolated Gelato subgraphs ready and deployed on Mainnet & Rinkeby, however @pimato mentioned that for the Omen-Exchange UI integration it would be better to have only one subgraph that contains every query for all the various underlying protocols, as adding multiple different ones using Apollo can create added complexity. Thus we made this PR.

I think from a UI integrations perspective @pimato is right about that. Also we will submit a PR to the omen-exchange repo soon that will have a functioning integration based on this commit of the subgraph.

@pimato your take?

@pimato
Copy link
Contributor

pimato commented Dec 21, 2020

I'm kind of confused actually. This part of the subgraph looks completely self-contained. Couldn't it just exist as a separate Gelato subgraph? What's the value in bundling this code along with the Omen subgraph?

@cag so that is actually what we started with and we also have isolated Gelato subgraphs ready and deployed on Mainnet & Rinkeby, however @pimato mentioned that for the Omen-Exchange UI integration it would be better to have only one subgraph that contains every query for all the various underlying protocols, as adding multiple different ones using Apollo can create added complexity. Thus we made this PR.

I think from a UI integrations perspective @pimato is right about that. Also we will submit a PR to the omen-exchange repo soon that will have a functioning integration based on this commit of the subgraph.

@pimato your take?

yup, you explained it better than I could.

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.

5 participants