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

Use Aragon Connect in dao apps command #1731

Merged
merged 3 commits into from
Jun 23, 2020
Merged

Use Aragon Connect in dao apps command #1731

merged 3 commits into from
Jun 23, 2020

Conversation

macor161
Copy link
Contributor

🦅 Pull Request Description

  • Use Aragon Connect to fetch apps when possible (i.e. on mainnet and Rinkeby)

🚨 Test instructions

dao apps <DAO address> --env aragon:rinkeby

Copy link
Contributor

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

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

Hey, I will test it tomorrow but wanted to say that I'm super happy to see that the change is that small!! 🙌
Thanks a lot for looking into it Mathew 😁

* @param {number} Ethereum network id
*/
const supportsAragonConnect = (networkId) => {
return networkId === 1 || networkId === 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking aloud here: the only reason we would want to continue using aragon.js going into the future is for local development environments (e.g. with buidler), because we wouldn't want to force them to run a graph node locally?

This is a compelling reason for us to eventually build that ethereum connector that we can fallback to if a subgraph is not available / does not contain this org (for whatever reason).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep we can likely remove aragon.js once we stop supporting the local devchain in aragonCLI.

Copy link
Contributor

Choose a reason for hiding this comment

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

because we wouldn't want to force them to run a graph node locally

@sohkai you are right it is a good reason. On the other hand, we realize working on a couple of Aragon apps subgraphs that it was esier to a developer using The Graph playground infrastructure directly and just have a staging subgraph that using the graph node locally. I think the main downside of this approach is relying heavily on The Graph infrastructure, but we were able to go a long way with it so far.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, the problem with this (for the buidler environment), is that we most likely don't want the user to run their own subgraph infrastructure for their local chain.

Copy link
Contributor

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

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

Working great! I'm still surprised of how little the change was 🔥

@macor161 macor161 merged commit 651d688 into master Jun 23, 2020
@macor161 macor161 deleted the aragon-connect-apps branch June 23, 2020 23:50
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