Skip to content

Conversation

@cgewecke
Copy link
Member

@cgewecke cgewecke commented Nov 30, 2017

PR just moves modifier detection logic added in #159 from the instrumentation step to a separate pre-processing step applied to all files in the contracts folder. This is because it's possible that files excluded from coverage might be used in the tests. See #146 - Zeppelin tests bug - for more.

(More or less) preserves the efficiency of #159.

Will test this on Zeppelin before merging.

(Tests are failing non-deterministically for some reason....they worked on rebuild though. Hmmm.)

@codecov-io
Copy link

codecov-io commented Nov 30, 2017

Codecov Report

Merging #161 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #161      +/-   ##
==========================================
- Coverage   97.14%   97.09%   -0.05%     
==========================================
  Files           6        6              
  Lines         385      379       -6     
  Branches       88       86       -2     
==========================================
- Hits          374      368       -6     
  Misses         11       11
Impacted Files Coverage Δ
lib/parse.js 93.87% <ø> (-0.25%) ⬇️
lib/coverageMap.js 100% <ø> (ø) ⬆️
lib/instrumentSolidity.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a13575...1645e34. Read the comment docs.

@cgewecke
Copy link
Member Author

cgewecke commented Dec 8, 2017

Good news: Zeppelin is passing on this PR.

Bad news: it's brute force double compilation and abi-swapping.

Reason: To modify the ABIs successfully we'd need to implement a recursive search of inherited contracts and keep track of their modifiers. For example:

BasicTokenMock is BasicToken
BasicToken is StandardToken
StandardToken is ERC20Token
etc...

solc will collate everything into the BasicTokenMock ABI, we'd have to do something similar to successfully change what needs to be changed. Possible but also entails adding a lot of logic etc.

@cgewecke cgewecke merged commit 11a8d7b into master Dec 8, 2017
@cgewecke cgewecke deleted the modify-all-abis branch December 8, 2017 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants