Update to Truffle 4.1.5 and Ganache 6.1.0

[frangio]

Resolves #800 by building on it and fixing the pending review comments there. Thanks @benjamincburns!
Resolves #863 by including the relevant commit here. Thanks @DumasV!

Fixes #868.

🚀 Description

Updates to Truffle 4.1.5 (includes update to Solidity 0.4.21), Ganache 6.1.0, and solidity-coverage 0.4.15 (necessary for newer syntax).

There are quite a lot of changes to package-lock.json. I generated it by copying the lockfile from master and running npm install with the new package.json.

The Solidity update brings a lot of new warnings that we should tackle before the next release. Those related to the emit keyword are fixed in this PR.

Had to change the MerkleProof tests to run through a contract wrapper instead of testing against the library contract directly (better explained in this PR's comments). And additionally I made MerkleProof.verifyProof an internal function because I think it makes sense? But it may be controversial.

• 📘 I've reviewed the OpenZeppelin Contributor Guidelines
• ✅ I've added tests where applicable to test my new functionality.
• 📖 I've made sure that my contracts are well-documented.
• 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

[frangio]

The MerkleProof tests are not passing in the coverage analysis. I'm not sure but they may be running out of gas.

[frangio]

@cgewecke How can I go about debugging tests that normally pass but don't do so with solidity-coverage? In this case it's the MerkleProof tests. The error is just a revert.

[shrugs]

I did some (terrible) grepping to make sure that all of the events were found and I didn't find any missing so this looks good to me 👍

if anyone wants to test themselves, you can find all of the events with something like

grep -P "\s+event [A-Z].+\(.+\);" ./**/*.sol

and then construct a regex like

grep -P "\s+(TargetCreated|TokenPurchase|Refunded|Sent|Received|Received|Show|HeirChanged|OwnerHeartbeated|OwnerProclaimedDead|HeirOwnershipClaimed|OwnershipTransferred|WhitelistedAddressAdded|WhitelistedAddressRemoved|RoleAdded|RoleRemoved|Burn|Approval|Transfer|Mint|Released|Transfer|Approval|ApprovalForAll)\(" ./**/*.sol

and then manually verify that they've all got emit or event before them (excluding the ones from node_modules that are part of solidity-coverage)

[shrugs]

I was just introduced to https://0xproject.com/docs/sol-cov via OpenZeppelin#zeppelin_os; it might be a reasonable approach if solidity-coverage is breaking on this upgrade.

We'd have to add a custom provider to the coverage network, but that's about it as far as I can tell.

[cgewecke]

@frangio I will look into this this weekend. Am planning a version upgrade that preprends emit to the instrumentation events to remove any warnings from 0.4.21. Will test against this PR and see what's going wrong with the merkle proof test.

@shrugs sol-cov is really well executed and uses an in-memory opcode trace to generate the coverage mapping. Ultimately, this is the direction any coverage tool in this space needs to move if it's going to be sustainable.

It would be great to run it against Zeppelin which is large and has an existing coverage measurement in order to get a sense of it's accuracy and usability and propose improvements (if any) etc. Would strongly encourage that - may merit an issue here or over at 0xProject where findings are listed.

[cgewecke]

@frangio The problem with MerkleProof is caused by coverage event injection, but this might be a solc bug. Since 0.4.20 any library method that fires an event now reverts. For example, if I strip MerkleProof down to:

contract MerkleProof {
  event LibraryEvent();

  function verifyProof() public {
    LibraryEvent();
  }
} 

and call it in a regular truffle test (no solidity-coverage):

await merkleProof.verifyProof();

I get revert.

Running the above in Remix, with the JS vm:

• 0.4.21: revert
• 0.4.20: revert
• 0.4.19: success

Do you know anything about this? You follow solidity more closely than I do - can't find anything in the release notes that suggests this change is expected.

Coverage will pass if you change MerkleProof from library to contract. Obviously wrong but idk.

[EDIT] - Also notice that there are a couple other libraries like ECRecover that aren't having this problem. They're intermediated by mocks, so one solution might be to make a mock that consumes MerkleProof?

[frangio]

@cgewecke Aha! What changed from 0.4.19 to 0.4.20 was the following:

Prevent non-view functions in libraries from being called directly (as opposed to via delegatecall).

That was after the Parity wallet selfdestruct fiasco.

So the problem is not that the library is emitting an event, but that the test is calling the library directly and not through a wrapper.

Thanks for figuring this out! Apologies that the problem was not really solidity-coverage.

[cgewecke]

@frangio Oh excellent - thanks for the explanation, that makes sense :)

[frangio]

Okay I fixed it by making MerkleProof tests run through a contract wrapper instead of testing the library directly. And additionally I made MerkleProof.verifyProof an internal function because I think it makes sense? (And it's easier to wrap it that way because there's no linking necessary.) But it may be controversial.

@shrugs Would you mind reviewing these changes please?

[shrugs]

another round of grepping

grep -r -P "pragma solidity" --exclude-dir node_modules --include=*.sol

ensures that all of the pragmas are good

the library change makes sense to me, too.