Use buidler's RC for compiling and testing #1071

fvictorio · 2020-09-21T23:41:07Z

Hi! This PR upgrades Buidler (and buidler-truffle5) to the RC of the next major version. This should let you remove the need of an extra setup for your legacy contracts.

I adapted the scripts so that contracts can be compiled and tests can be executed. I haven't udpated the coverage scripts because that plugin is not adapted to the new compilation pipeline yet.

You'll see a lot of lines modified in the tests. This is because one of the changes in buidler's new compilation pipeline is that artifacts don't live in a flat structure in the artifacts directory anymore. Instead, they replicate the directory structure of the contracts. This means that now you can have contracts with the same name, without one artifact being written over the other.

But this comes with a cost: now if you have two contracts named Foo you can't do artifacts.require('Foo'), because there's no way to know which one you want. In those scenarios, you have to use the Fully Qualified Name, for example contracts/Foo.sol:Foo. When there's only one contract with that name, this is not necessary.

Most tests seem to pass with the new setup. Some of them failed, but I think it wasn't unrelated to this change (timeouts for example). But of course let me know if this indeed breaks some tests.

You surely do other things in your workflow besides compiling and running tests, and chances are they don't work with this new setup. Please let me know when that happens and I'll try to help!

Hope you enjoy it 😄

Anyhowclick · 2020-09-22T07:07:51Z

Haha woah, thanks for the PR!

Loving the feature! =)

ilanDoron

Thanks very much for this PR

its a great feature.

2 comments:

file 'contracts/sol5/Token.sol' can be deleted. its code should be copied as is to "contracts/sol5/bridges/eth2Dai/mock/WethToken.sol"

the above will enable removing all places where it has this specific artifact used:
'const TestToken = artifacts.require("contracts/sol6/mock/Token.sol:Token");'
I can PR this to your code if its too much hassle.

travis checks are failing due to js heap size limit reached. can this change be related to new buidler feature?

fvictorio · 2020-09-22T12:46:30Z

I can PR this to your code if its too much hassle.

I can do it, no problem. But anyway you should be able to push to my branch if you want.

travis checks are failing due to js heap size limit reached. can this change be related to new buidler feature?

It's possible that it's using more memory than before, but it shouldn't be a big difference. Maybe I should add export NODE_OPTIONS=--max-old-space-size=4096 to the tst.sh script, like cmp.sh has?

ilanDoron · 2020-09-22T12:49:15Z

Thanks Regarding pushing size export to tst.sh. might try not sure though

…

On Tue, Sep 22, 2020 at 3:46 PM Franco Victorio ***@***.***> wrote: I can PR this to your code if its too much hassle. I can do it, no problem. But anyway you should be able to push to my branch if you want. travis checks are failing due to js heap size limit reached. can this change be related to new buidler feature? It's possible that it's using more memory than before, but it shouldn't be a big difference. Maybe I should add export NODE_OPTIONS=--max-old-space-size=4096 to the tst.sh script, like cmp.sh has? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1071 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACNXTBNXENZVHZQ3CWIUEETSHCMDLANCNFSM4RVDRTBA> .

fvictorio · 2020-09-22T13:07:41Z

I ended up removing just the Token of Token.sol and inlining that in WethToken.sol, and then I renamed Token.sol to StandardToken.sol. I did it that way just to avoid moving a lot of stuff to WethToken. In any case, it did work to avoid having to use the FQN in the tests.

ilanDoron · 2020-09-22T18:17:04Z

buidler.config.js

  },

  paths: {
-    sources: "./contracts/sol6",
+    sources: "./contracts",
    tests: "./test/",
  },

  mocha: {
    enableTimeouts: false


I see some tests time out now
seems this flag not working?

Apparently that option is not working anymore. That's weird because, while enableTimeouts was removed in mocha@8, buidler depends on version 7.

You can use timeout: 0 instead.

Oh
I see
thanks
can u update here
it fails the CI for this PR

ilanDoron

Timeout issue was solved
great
trying to look at the failures locally

when compiling localy I see these results:

Compiling with 0.4.18
Downloading compiler version 0.4.18
Compiled 65 contracts successfully
Compiling with 0.4.18
Compiled 1 contract successfully
Compiling with 0.4.18
Compiled 7 contracts successfully
Compiling with 0.4.18
Compiled 1 contract successfully
Compiling with 0.4.18
Compiled 1 contract successfully
Compiling with 0.4.18
Compiled 1 contract successfully
Compiling with 0.4.18
Compiled 1 contract successfully
Compiling with 0.4.18
Compiled 1 contract successfully
Compiling with 0.4.18
Compiled 1 contract successfully
Compiling with 0.4.18
Compiled 1 contract successfully
Compiling with 0.4.18
Compiled 1 contract successfully
Compiling with 0.4.18
Compiled 1 contract successfully
Compiling with 0.5.11
Downloading compiler version 0.5.11
Compiled 13 contracts successfully
Compiling with 0.5.11
Compiled 3 contracts successfully
Compiling with 0.6.6
Downloading compiler version 0.6.6
Compiled 102 contracts successfully
Compiling with 0.6.6
Compiled 1 contract successfully
Compiling with 0.6.6
Compiled 1 contract successfully
Compiling with 0.6.6
Compiled 1 contract successfully
Compiling with 0.6.6
Compiled 1 contract successfully

this expected behaviour?

fvictorio · 2020-09-23T20:18:21Z

Oh, yeah, the output is ugly right now. It'll get better!

ilanDoron · 2020-09-24T06:04:14Z

@fvictorio
I am running locally
seems the EVM behaviour changed a bit for assert.

4 tests that cause assert to trigger are now returning invalid opcode and not revert opcode.

we use openzeppelin expect revert code and it issues an error.
any idea about this?

ilanDoron · 2020-09-24T06:04:59Z

Oh, yeah, the output is ugly right now. It'll get better!

OK
we can leave with that
:)

fvictorio · 2020-09-24T13:59:45Z

4 tests that cause assert to trigger are now returning invalid opcode and not revert opcode.

Yes, I forgot to mention that! This changed but I think the previous behavior should be considered a bug, so changing the assertions should be fine AFAIK. @alcuadrado is this correct?

alcuadrado · 2020-09-24T17:05:51Z

That is correct, @fvictorio

ilanDoron · 2020-09-24T18:31:19Z

Meaning that new evm is more aligned with expected evm behavior?

…

On Thu, 24 Sep 2020, 20:06 Patricio Palladino, ***@***.***> wrote: That is correct, @fvictorio <https://github.com/fvictorio> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1071 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACNXTBILKSLJQNIJLC6SKSTSHN375ANCNFSM4RVDRTBA> .

alcuadrado · 2020-09-24T18:55:03Z

Yes, exactly @ilanDoron. The previous behavior was a bug, which prevented you to distinguish between tx failing because of revert and invalid opcodes been executed. My bug actually 😅

ilanDoron · 2020-09-25T11:19:39Z

OK great Big thanks will update our tests.

…

On Thu, Sep 24, 2020 at 9:55 PM Patricio Palladino ***@***.***> wrote: Yes, exactly @ilanDoron <https://github.com/ilanDoron>. The previous behavior was a bug, which prevented you to distinguish between tx failing because of revert and invalid opcodes been executed. My bug actually 😅 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1071 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACNXTBIJUHPYJK3UPDTLUTLSHOIZNANCNFSM4RVDRTBA> .

- keep require call for Token.sol with current convetion. to be aliged with other calls.

ilanDoron · 2020-09-25T18:12:15Z

@fvictorio please see my PR to your code.
should solve the failing tests.
And also a few more updates.

ilanDoron · 2020-09-29T12:32:01Z

@fvictorio
please approve here:
fvictorio#1

fvictorio · 2020-09-29T13:25:22Z

Hey @ilanDoron, I left a comment in that PR with respect to the yarn.lock

Updating tests to have correct check when expecting assertion. and reverting some changes in require statments

import Token.sol as other files. a few more files.

ilanDoron · 2020-09-30T08:26:46Z

@fvictorio
I apologise
had a typo
please merge once more

Fix Bad include name

fvictorio added 2 commits September 21, 2020 20:31

Install RC version of buidler with multi-solc and adapt scripts

99a16be

Disambiguate artifact names in tests

40f4c0a

ilanDoron reviewed Sep 22, 2020

View reviewed changes

fvictorio added 5 commits September 22, 2020 10:01

Fix getopts in tst.sh

529b8e0

Increase heap size for tests

f740c18

Inline contracts/sol5/mock/Token.sol in WethToken and rename file

90d70be

Use "Token" in tests instead of FQN

9a90b97

Fix solhint errors

299b0ca

Increase heap size for travis test script

e6fde55

ilanDoron reviewed Sep 22, 2020

View reviewed changes

Use timeout instead of enableTimeouts

2de2196

ilanDoron reviewed Sep 23, 2020

View reviewed changes

- update test helper for correct testing of assert

12ccc69

- keep require call for Token.sol with current convetion. to be aliged with other calls.

ilanDoron and others added 2 commits September 29, 2020 21:09

remove yarn.lock

1da8501

Merge pull request #1 from ilanDoron/multi-solc

c2c2444

Updating tests to have correct check when expecting assertion. and reverting some changes in require statments

ilanDoron and others added 3 commits September 29, 2020 22:42

import Token.sol as other files.

bf3d2aa

Merge pull request #2 from ilanDoron/multi-solc

fa4a2ad

import Token.sol as other files. a few more files.

Fix Bad include name

3d6c850

Merge pull request #3 from ilanDoron/multi-solc

009a12c

Fix Bad include name

Anyhowclick approved these changes Oct 5, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use buidler's RC for compiling and testing #1071

Use buidler's RC for compiling and testing #1071

fvictorio commented Sep 21, 2020

Anyhowclick commented Sep 22, 2020

ilanDoron left a comment •

edited

Loading

fvictorio commented Sep 22, 2020

ilanDoron commented Sep 22, 2020 via email

fvictorio commented Sep 22, 2020 •

edited

Loading

ilanDoron Sep 22, 2020

fvictorio Sep 22, 2020

ilanDoron Sep 23, 2020

ilanDoron left a comment

fvictorio commented Sep 23, 2020

ilanDoron commented Sep 24, 2020 •

edited

Loading

ilanDoron commented Sep 24, 2020

fvictorio commented Sep 24, 2020

alcuadrado commented Sep 24, 2020

ilanDoron commented Sep 24, 2020 via email

alcuadrado commented Sep 24, 2020

ilanDoron commented Sep 25, 2020 via email

ilanDoron commented Sep 25, 2020

ilanDoron commented Sep 29, 2020

fvictorio commented Sep 29, 2020 •

edited

Loading

ilanDoron commented Sep 30, 2020

Use buidler's RC for compiling and testing #1071

Are you sure you want to change the base?

Use buidler's RC for compiling and testing #1071

Conversation

fvictorio commented Sep 21, 2020

Anyhowclick commented Sep 22, 2020

ilanDoron left a comment • edited Loading

Choose a reason for hiding this comment

fvictorio commented Sep 22, 2020

ilanDoron commented Sep 22, 2020 via email

fvictorio commented Sep 22, 2020 • edited Loading

ilanDoron Sep 22, 2020

Choose a reason for hiding this comment

fvictorio Sep 22, 2020

Choose a reason for hiding this comment

ilanDoron Sep 23, 2020

Choose a reason for hiding this comment

ilanDoron left a comment

Choose a reason for hiding this comment

fvictorio commented Sep 23, 2020

ilanDoron commented Sep 24, 2020 • edited Loading

ilanDoron commented Sep 24, 2020

fvictorio commented Sep 24, 2020

alcuadrado commented Sep 24, 2020

ilanDoron commented Sep 24, 2020 via email

alcuadrado commented Sep 24, 2020

ilanDoron commented Sep 25, 2020 via email

ilanDoron commented Sep 25, 2020

ilanDoron commented Sep 29, 2020

fvictorio commented Sep 29, 2020 • edited Loading

ilanDoron commented Sep 30, 2020

ilanDoron left a comment •

edited

Loading

fvictorio commented Sep 22, 2020 •

edited

Loading

ilanDoron commented Sep 24, 2020 •

edited

Loading

fvictorio commented Sep 29, 2020 •

edited

Loading