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

Server: add wrapper to fetch data from the graph #59

Merged
merged 8 commits into from
Oct 19, 2020
Merged

Conversation

0xGabi
Copy link
Contributor

@0xGabi 0xGabi commented Oct 16, 2020

Reuse the scheme we were using on Connect to have a basic wrapper that only supports query data from the graph.

@0xGabi 0xGabi added the S1: Feature 👨‍🎨 Adding new stuff label Oct 16, 2020
@0xGabi 0xGabi requested review from bpierre and nivida October 16, 2020 00:48
@0xGabi 0xGabi added this to In progress in Aragon Govern v1.0 via automation Oct 16, 2020
Copy link
Contributor

@nivida nivida left a comment

Choose a reason for hiding this comment

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

Added some comments. I think many of them are just popped up cause I'm not sure what the standards are here at Aragon.

Overall: I would probably combine those two TheGraph classes to a TheGraphServiceClient. 🤷‍♂️

packages/govern-server/src/core/data/index.ts Show resolved Hide resolved
packages/govern-server/src/core/errors.ts Show resolved Hide resolved
@@ -0,0 +1,84 @@
import { ErrorInvalidNetwork } from '../errors'
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how the structure is but we will use I think anyways all of those functions here. This means it will get bundled anyways and we could forward the creation of nice TS classes.

What do you think about creating out of it a value object class?

const network = new Network('mainnet');
console.log(network.chainId);

const network = new Network(netObj);
console.log(network.chainId);

console.log(network.toObject());
> { ... }

packages/govern-server/src/core/data/index.ts Show resolved Hide resolved
@bpierre bpierre mentioned this pull request Oct 16, 2020
@izqui izqui added this to the Beta X: Mainnet pre-release milestone Oct 19, 2020
@codecov-io
Copy link

codecov-io commented Oct 19, 2020

Codecov Report

Merging #59 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #59   +/-   ##
=======================================
  Coverage   36.76%   36.76%           
=======================================
  Files           8        8           
  Lines         204      204           
  Branches       30       30           
=======================================
  Hits           75       75           
  Misses        129      129           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9767db...5f82ff2. Read the comment docs.

@bpierre bpierre marked this pull request as ready for review October 19, 2020 10:24
@bpierre bpierre merged commit 1853fa6 into master Oct 19, 2020
Aragon Govern v1.0 automation moved this from In progress to Done Oct 19, 2020
@bpierre bpierre deleted the thegraph-data branch October 19, 2020 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S1: Feature 👨‍🎨 Adding new stuff
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants