smart-contracts
smart-contracts copied to clipboard
KyberNetwork
Use buidler's RC for compiling and testing
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 :smile:
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?
Thanks
Regarding pushing size export to tst.sh. might try not sure though
On Tue, Sep 22, 2020 at 3:46 PM Franco Victorio [email protected] 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 https://github.com/KyberNetwork/smart-contracts/pull/1071#issuecomment-696697543, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACNXTBNXENZVHZQ3CWIUEETSHCMDLANCNFSM4RVDRTBA .
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.
@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?
Oh, yeah, the output is ugly right now. It'll get better!
OK we can leave with that :)
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?
Meaning that new evm is more aligned with expected evm behavior?
On Thu, 24 Sep 2020, 20:06 Patricio Palladino, [email protected] 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 https://github.com/KyberNetwork/smart-contracts/pull/1071#issuecomment-698470990, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACNXTBILKSLJQNIJLC6SKSTSHN375ANCNFSM4RVDRTBA .
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 😅
OK great Big thanks will update our tests.
On Thu, Sep 24, 2020 at 9:55 PM Patricio Palladino [email protected] 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 https://github.com/KyberNetwork/smart-contracts/pull/1071#issuecomment-698526240, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACNXTBIJUHPYJK3UPDTLUTLSHOIZNANCNFSM4RVDRTBA .
@fvictorio please see my PR to your code. should solve the failing tests. And also a few more updates.
@fvictorio please approve here: https://github.com/fvictorio/smart-contracts/pull/1
Hey @ilanDoron, I left a comment in that PR with respect to the yarn.lock