Migrate to Buidler plugin #1131

sohkai · 2020-04-14T10:21:47Z

This PR introduces a vanilla setup of buidler development tool. It includes several network configurations both for local development and interacts with real chains. It also has ganache configured to be use as an alternative local development chain (use with --network ganache).

Note the gas reporter packages don't yet support handling the start and stop of the chain. We need to start it manually for now as explained in https://github.com/cgewecke/buidler-gas-reporter/tree/master#using-buidlerevm-warning.

TODO:

Support buidlerevm as default test chain. Depends on Update assertRevert to support buidler dev tool contract-helpers#55.
Review skipped test. Seems it was affected by the Byzantium hard fork
Make coverage work, update to latest solidity-coverage
Take another look at the .gitignore / other configuration files
Update truffle-extract to support buidler. As discussed in Migrate to Buidler plugin #1131 (comment).

Move to address on a future PR:

Migrate smart contract lint rules from .solium to .solhint (afterwards we should remove .soliumrc.json)

apps/vault/test/vault_shared.js

sohkai

Couple more notes, mainly about using ganache.

The failing tests seem to point to ganache or the truffle integration with buidler being an issue, as the transaction or receipts aren't being handled well (testing with ganache seems to be really slow).

I'm not really sure what's going on, but somehow the coverage plugin is using ganache and:

Is faster than vanilla ganache?
Works correctly 🤷

Since all of these test environments are backed by ethereumjs-vm anyway, I think we'd only be able to hedge implementation risk by running tests against a real geth node anyway.

Finally, with solidity-coverage 0.7.x, I think we should be able to remove a bunch of options and/or use the new skip syntax. See upgrade nodes (note that some of these won't apply anymore because we can use the buidler plugin, which does a lot of this automatically).

apps/agent/package.json

apps/agent/buidler.config.js

sohkai · 2020-08-11T09:37:55Z

apps/agent/package.json

-    "test:gas": "GAS_REPORTER=true npm test",
-    "coverage": "npx truffle run coverage",
-    "ganache-cli:test": "./node_modules/@aragon/contract-helpers-test/scripts/ganache-cli.sh",
-    "abi:extract": "truffle-extract --output abi/ --keys abi",


Right, we may want to replace this with something else. Not sure if there's a buidler plugin to replicate this, but truffle-extract supports a buildDir input argument so we can switch to artifacts.

Maybe we can update it to be a super simple plugin? Otherwise, if I update this line as well: https://github.com/sohkai/truffle-extract/blob/master/index.js#L36 with npx buidler compile the package keep working fine. I don't have a strong preference, do you?

I decide to fork your package and publish a new version for buidler instead: https://github.com/0xGabi/buidler-extract

Sounds good! Honestly not sure if this fits well as a small buidler plugin, but I can see it being useful!

I was thinking about this earlier, and in reality, we may actually want to have a tool that lets us configure what we publish. For most apps, there's a lot of extra fluff in the artifacts (e.g. Voting only needs Voting.json, not all the other artifacts).

apps/agent/scripts/ganache-cli.sh

.gitignore

sohkai · 2020-08-11T21:30:53Z

apps/agent/package.json

-    "coverage": "SOLIDITY_COVERAGE=true npm run ganache-cli:test",
-    "ganache-cli:test": "./scripts/ganache-cli.sh"
+    "test": "buidler test",
+    "test:gas": "REPORT_GAS=true buidler test --network localhost",


Not sure, but I don't think the localhost network exists anymore in the buidler config?

As we are using the gas reporter with the buidlerevm and they don't yet handle the start and stop of the chain. We need to manually ran npx buidler node and then connect to the chain with --network localhost. As explained on:

https://github.com/cgewecke/buidler-gas-reporter/tree/master#using-buidlerevm-warning

https://buidler.dev/buidler-evm/#connecting-to-buidler-evm-from-wallets-and-other-software

I include a comment to give more context about it: 1114631#diff-31b24dc429d79c5898460ef28314ee00R38

Gotcha, so the --network local flag would tell it to connect to the separately run buidlerevm instance?

Yes. The buidlerevm chain starts a JSON-RPC and WebSocket server and expose the gateway at http://localhost:8545. Then the buidler default configuration object has the following format:

{ localhost: { url: "http://127.0.0.1:8545" }, buidlerevm: { // See its defaults } }

That's why you can use --network localhost. Maybe we should be explicit about localhost network?

apps/agent/buidler.config.js

sohkai · 2020-08-13T10:12:09Z

apps/agent/package.json

-    "coverage": "SOLIDITY_COVERAGE=true npm run ganache-cli:test",
-    "ganache-cli:test": "./scripts/ganache-cli.sh"
+    "test": "buidler test",
+    "test:gas": "REPORT_GAS=true buidler test --network localhost",


Gotcha, so the --network local flag would tell it to connect to the separately run buidlerevm instance?

Co-authored-by: Brett Sun <[email protected]>

Add ganache as default network and include a few comments on each network

coveralls · 2020-08-13T18:25:14Z

Coverage increased (+0.2%) to 97.554% when pulling d0c0991 on buidler into 7a2ae95 on master.

0xGabi · 2020-08-13T19:52:43Z

We have a green CI 🎉

@sohkai if you consider it's ok, we can merge.

sohkai

Amazing, this is fantastic! Let's get this innnn 🍾!

sohkai · 2020-08-14T14:46:09Z

apps/agent/test/agent_shared.js

@@ -734,11 +734,14 @@ module.exports = (agentName, { accounts, artifacts, web3 }) => {
        context('> Designated signer', () => {
          const ethSign = async (hash, signer) => {
            const packedSig = await web3.eth.sign(hash, signer)
+            const chainId = await web3.eth.net.getId()
+            // If we detect buidlerevm we don't use ganache encoding
+            const offset = chainId === 31337 ? 0 : 27


Ohhh interesting, are there docs for this? I'd like to leave a few more comments :).

We may also want to push something like this into the contract-helpers (we can do it later though)

This was pointing out by Pato from buidler. He told me that ganache encodes the v value of the signature as 0 or 1, while it should be 27 or 28. But buidler uses the same values as geth.

Related discussions about the topic:

Add helpers for new eth_sign behavior ethereumjs/ethereumjs-util#67 (comment)

eth_sign API out of sync with ethapi trufflesuite/ganache#555

Interesting, let's leave these in the inline code!

apps/voting/.solcover.js

This PR introduces a vanilla setup of buidler development tool. It includes several network configurations both for local development and to interacts with real chains. It also has ganache configured to be used as an alternative local development chain (use with `--network ganache`). Note the gas reporter packages don't yet support handling the start and stop of the chain. We need to start it manually as explained in https://github.com/cgewecke/buidler-gas-reporter/tree/master#using-buidlerevm-warning. The changes include: - Support buidlerevm as default test chain. Depends on aragon/contract-helpers#55. - Make coverage work with buidler and update to latest solidity-coverage - Bump to use node 12 - Update `truffle-extract` to support buidler. As discussed in aragon/aragon-apps#1131 (comment).

This PR introduces a vanilla setup of buidler development tool. It includes several network configurations both for local development and to interacts with real chains. It also has ganache configured to be used as an alternative local development chain (use with `--network ganache`). Note the gas reporter packages don't yet support handling the start and stop of the chain. We need to start it manually as explained in https://github.com/cgewecke/buidler-gas-reporter/tree/master#using-buidlerevm-warning. The changes include: - Support buidlerevm as default test chain. Depends on aragon/contract-helpers#55. - Make coverage work with buidler and update to latest solidity-coverage - Bump to use node 12 - Update `truffle-extract` to support buidler. As discussed in aragon#1131 (comment).

This PR introduces a vanilla setup of buidler development tool. It includes several network configurations both for local development and to interacts with real chains. It also has ganache configured to be used as an alternative local development chain (use with `--network ganache`). Note the gas reporter packages don't yet support handling the start and stop of the chain. We need to start it manually as explained in https://github.com/cgewecke/buidler-gas-reporter/tree/master#using-buidlerevm-warning. The changes include: - Support buidlerevm as default test chain. Depends on aragon/contract-helpers#55. - Make coverage work with buidler and update to latest solidity-coverage - Bump to use node 12 - Update `truffle-extract` to support buidler. As discussed in aragon/aragon-apps#1131 (comment).

probot-autolabeler bot added app: token manager app: vault component: contract component: frontend labels Apr 14, 2020

sohkai commented Apr 14, 2020

View reviewed changes

apps/vault/test/vault_shared.js Outdated Show resolved Hide resolved

Agent: Move to buidler

6d9323e

probot-autolabeler bot added app: agent app: finance labels Aug 6, 2020

0xGabi added 7 commits August 6, 2020 19:44

Update imports and buidler config

62a7601

Shared: Update config files and remove shared migrations

977ca67

Agreements: move to buidler

ee2d7ee

Finance: move to buidler

3e31401

Tokens: move to buidler

bbe7170

Vault: move to buidler

c33cc9d

Voting: move to buidler

ef80ab6

0xGabi force-pushed the buidler branch from 2753377 to ef80ab6 Compare August 6, 2020 22:49

0xGabi added 9 commits August 10, 2020 18:00

Agent: Use vanilla buidler setup (#1245)

16ec973

Agreements: Vanilla buidler setup

bba0749

Finance: Vanilla buidler setup

779a903

Tokens: Vanilla buidler setup

7309046

Vault: Vanilla buidler setup

4988db0

Voting: Vanilla buidler setup

c7c88dd

Update yarn.lock

75d056d

CI: Update node to version 12

c2d2283

Chore: Merge with master app yarn.lock files

093ccf7

0xGabi marked this pull request as ready for review August 10, 2020 21:58

auto-assign bot requested review from bpierre and Evalir August 10, 2020 21:58

sohkai commented Aug 11, 2020

View reviewed changes

sohkai mentioned this pull request Aug 11, 2020

Create ganache-cli-buidler.sh aragon/contract-helpers#54

Closed

0xGabi added 2 commits August 11, 2020 14:40

Use new package buidler-extract

0a823a6

Chore: Bump contract-helpers-tests version

ee83bd3

sohkai commented Aug 11, 2020

View reviewed changes

0xGabi added 2 commits August 12, 2020 14:32

Chore: Add network configurations

1114631

Add ganache as default chian using buidler plugin

9229b69

sohkai commented Aug 13, 2020

View reviewed changes

0xGabi and others added 10 commits August 13, 2020 11:47

Chore: Add ganache url to network config

ebf3423

Update apps/agent/buidler.config.js

12cca7b

Co-authored-by: Brett Sun <[email protected]>

Chore: Network configurations

c0bee8c

Add ganache as default network and include a few comments on each network

Merge branch 'buidler' of github.com:aragon/aragon-apps into buidler

840cbd1

Chore: Apply review typo fixes

4b6cba9

Chore: Fix configuration object order

ce18052

Chore: Add missing usePlugin import

cb7d0ab

Fix: remove mocha only flag on tests

2c2548c

Test: Use buidlerevm as default

03d630b

Tests: Add flag to apply ganache encoding offset on signature

fbf1b9e

0xGabi added 2 commits August 13, 2020 15:27

Test: Skip tests with gas diffrence on coverage

32e4e28

Tests: Config skipFiles rule

d0c0991

sohkai commented Aug 14, 2020

View reviewed changes

Docs: Add inline comment on test

5c78264

0xGabi merged commit d631fd6 into master Aug 14, 2020

0xGabi deleted the buidler branch August 14, 2020 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to Buidler plugin #1131

Migrate to Buidler plugin #1131

sohkai commented Apr 14, 2020 •

edited by 0xGabi

Loading

sohkai left a comment •

edited

Loading

sohkai Aug 11, 2020 •

edited

Loading

0xGabi Aug 11, 2020 •

edited

Loading

0xGabi Aug 11, 2020

sohkai Aug 11, 2020

sohkai Aug 11, 2020

0xGabi Aug 12, 2020 •

edited

Loading

0xGabi Aug 12, 2020

sohkai Aug 13, 2020

0xGabi Aug 13, 2020 •

edited

Loading

sohkai Aug 13, 2020

coveralls commented Aug 13, 2020 •

edited

Loading

0xGabi commented Aug 13, 2020

sohkai left a comment

sohkai Aug 14, 2020

0xGabi Aug 14, 2020

sohkai Aug 14, 2020

Migrate to Buidler plugin #1131

Migrate to Buidler plugin #1131

Conversation

sohkai commented Apr 14, 2020 • edited by 0xGabi Loading

sohkai left a comment • edited Loading

Choose a reason for hiding this comment

sohkai Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

0xGabi Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xGabi Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xGabi Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 13, 2020 • edited Loading

0xGabi commented Aug 13, 2020

sohkai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai commented Apr 14, 2020 •

edited by 0xGabi

Loading

sohkai left a comment •

edited

Loading

sohkai Aug 11, 2020 •

edited

Loading

0xGabi Aug 11, 2020 •

edited

Loading

0xGabi Aug 12, 2020 •

edited

Loading

0xGabi Aug 13, 2020 •

edited

Loading

coveralls commented Aug 13, 2020 •

edited

Loading