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

Devops #4

Merged
merged 14 commits into from
Oct 29, 2018
Merged

Devops #4

merged 14 commits into from
Oct 29, 2018

Conversation

tsuberim
Copy link
Contributor

  • Generate abis from @daostack/arc automatically as part of codegen
  • Allow configuration of the project to mainnet/development using yaml templating.
  • Set up integration testing (unfortunately doesn't work right now due to bug in ganache)
  • Development with or without docker & docker-compose.

@tsuberim tsuberim self-assigned this Oct 24, 2018
@ben-kaufman
Copy link
Contributor

I think the ABIs should be fixed to the live version of the protocol when choosing mainnet.

package-lock.json
*.log
.env
subgraph.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant cr

.gitignore Outdated
subgraph.yaml
docker-compose.yml
config.json
abis
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant cr

@orenyodfat
Copy link
Contributor

@tsuberim which bug in ganache you are referring to ?

@tsuberim
Copy link
Contributor Author

@orenyodfat The bug is trufflesuite/ganache#907 which has been fixed but not yet released as a new version.

@tsuberim
Copy link
Contributor Author

@ben-kaufman just to be clear I only mean the ABI interface definitions (not the whole .json compiled file). Contract addresses are configured in ops/config.yaml. Since ABIs are the same regardless of the chain, I don't see what you mean.

@orenyodfat
Copy link
Contributor

orenyodfat commented Oct 24, 2018

@orenyodfat The bug is trufflesuite/ganache#907 which has been fixed but not yet released as a new version.

Is that effect graph-node code or the code added for this pr ? Is there a way to bypass that ?
@tsuberim

@orenyodfat
Copy link
Contributor

orenyodfat commented Oct 24, 2018

@ben-kaufman just to be clear I only mean the ABI interface definitions (not the whole .json compiled file). Contract addresses are configured in ops/config.yaml. Since ABIs are the same regardless of the chain, I don't see what you mean.

Please note that the deployment addresses can also potentially
be fetched out from the contract.json . (e.g arc.js package currently contain that though I guess it will be changed in the future ..need to discuss)
later on we can use this as part of the automate codegen (generate the yaml files)

@tsuberim
Copy link
Contributor Author

@orenyodfat

  1. It prevents us from running graph-node against ganache and thus running tests. I don't think there's a way to bypass it (maybe we should ask TheGraph team).
  2. Yes, that's true.

@orenyodfat
Copy link
Contributor

@tsuberim so it is basically a graph-node issue ? graphprotocol/graph-node#375 ?

@tsuberim
Copy link
Contributor Author

@orenyodfat No, its ganache not following the spec (not setting the to field to null for contract creation tx)

@nenadjaja
Copy link

Hi @orenyodfat I updated the issue graphprotocol/graph-node#375 with the steps to run ganache locally (that works with graph-node). It has the fix, that's not released yet. Hope it helps!

@ben-kaufman
Copy link
Contributor

@ben-kaufman just to be clear I only mean the ABI interface definitions (not the whole .json compiled file). Contract addresses are configured in ops/config.yaml. Since ABIs are the same regardless of the chain, I don't see what you mean.

I see that you import the ABI json files directly from Arc, while the version of the contract being used currently on the mainnet is different from the one in Arc, meaning the ABI is different than the ABI of the live contract.

@orenyodfat
Copy link
Contributor

@ben-kaufman just to be clear I only mean the ABI interface definitions (not the whole .json compiled file). Contract addresses are configured in ops/config.yaml. Since ABIs are the same regardless of the chain, I don't see what you mean.

I see that you import the ABI json files directly from Arc, while the version of the contract being used currently on the mainnet is different from the one in Arc, meaning the ABI is different than the ABI of the live contract.

Right . we need to put some thoughts on how to handle multiple versions of the same contracts (with different abi or not)

README.md Outdated
Develop locally:
## Configure the project

### .env variables
Copy link
Contributor

Choose a reason for hiding this comment

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

please add an .env template file

@orenyodfat
Copy link
Contributor

I guess it will be nice to add ganache-cli to package.json and have a script to run it with the mnemonic

@tsuberim
Copy link
Contributor Author

@orenyodfat
Copy link
Contributor

Should we add ganache-cli to package.json so it can be run manually regardless docker ?
please update readme on how to run development + tests

@tsuberim tsuberim force-pushed the devops branch 3 times, most recently from 07c7ee3 to 69ab80c Compare October 28, 2018 15:30
Copy link
Contributor

@orenyodfat orenyodfat left a comment

Choose a reason for hiding this comment

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

👍
Lets merge this pr and open issues if needed.


# Get started
DAOstack subgraph for [TheGraph](https://thegraph.com/) project.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a link to graph-node GitHub as theGraph


# Get started
DAOstack subgraph for [TheGraph](https://thegraph.com/) project.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "DAOstack subgraph using graph-node project."

@leviadam leviadam merged commit 234b43f into master Oct 29, 2018
@leviadam leviadam deleted the devops branch October 31, 2018 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants