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

Agent: Move to buidler #1245

Merged
merged 1 commit into from
Aug 10, 2020
Merged

Agent: Move to buidler #1245

merged 1 commit into from
Aug 10, 2020

Conversation

0xGabi
Copy link
Contributor

@0xGabi 0xGabi commented Aug 6, 2020

Replace #1092

This PR applies the changes to move the Agent app to use buidler as a developer tool and the Aragon Buidler plugin.

Changes are straight forward now that tests were migrated to truffle5. Once we get one of the apps merged I can move fast and replicate the same for the rest as is a mechanical process.

Note I also start using yarn as default. Let me know what you think and if I may rollback the change.

A few open questions we can address on following PRs are:

  1. Migrate solium rules to use solhint
  2. Decide about the rule we have on the node engine (currently we are fixed to node 10) with the migration to buidler I think we can safely move to node 12.

@@ -1,8 +1,12 @@
pragma solidity 0.4.24;

import "@aragon/os/contracts/acl/ACL.sol";
import "@aragon/os/contracts/apm/APMRegistry.sol";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new imports are needed to deploy the Aragon bases on the local ganache chain we ran with the buidler plugin.

@@ -0,0 +1,80 @@
#!/usr/bin/env bash
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file will live on @aragon/contract-helpers-test. I include it locally until we publish the new version.

@0xGabi 0xGabi changed the base branch from master to buidler August 6, 2020 20:27
@0xGabi 0xGabi changed the base branch from buidler to master August 6, 2020 20:28
Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

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

Hey @0xGabi, I must've miscommunicated a bit when discussing this migration!

Let me know what your thoughts are on my comments below!


Note I also start using yarn as default. Let me know what you think and if I may rollback the change.

Yes, this should be the case already! This repo now uses yarn workspaces!

Migrate solium rules to use solhint

We can do this in a second step :)

Decide about the rule we have on the node engine (currently we are fixed to node 10) with the migration to buidler I think we can safely move to node 12.

Agreed, I pinned it to node 10 because it still relied on [email protected].

const { usePlugin } = require('@nomiclabs/buidler/config')
const hooks = require('./scripts/buidler-hooks')

usePlugin('@aragon/buidler-aragon')
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than use the buidler-aragon plugin, I was wondering if we shouldn't move to a vanilla buidler set up for now? That way we might be able to use buidlerevm or other defaults rather than configure our own ganache chain / etc.

I'm not particularly concerned at the moment about providing a good frontend developer experience for others in these apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline, this is a good step forward.

apps/agent/arapp.json Outdated Show resolved Hide resolved
@0xGabi 0xGabi changed the base branch from master to buidler August 10, 2020 20:46
@0xGabi
Copy link
Contributor Author

0xGabi commented Aug 10, 2020

I'll merge with the main buidler branch where I will aggregate the rest of the apps changes.

@0xGabi 0xGabi merged commit 16ec973 into aragon:buidler Aug 10, 2020
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.

2 participants