Using solc to compare bytecode on specific contracts #461
Conversation
| // - A worker process has failed to exit gracefully and has been force | ||
| // exited. This is likely caused by tests leaking due to improper | ||
| // teardown. Try running with --detectOpenHandles to find leaks. | ||
| const compileContract = require('./utils/compile-contract'); |
There was a problem hiding this comment.
I'd appreciate if someone could investigate how to include the compiler, without incurring in greater testing times.
There was a problem hiding this comment.
There is a way to improve the time: download a native compiler and use that instead. It's significantly faster. But this means:
- Downloading the correct compiler according to the OS (and falling back to the WASM one if the OS is not supported)
- Avoiding a re-download if it was previously download
- (Optional) Cache it in the CI
I can do this since we do something similar (even more involved) in Hardhat.
That being said, it takes 1m to run all the tests with the AST and bytecode checks, so I don't think it's worth it? I'd rather re-visit this in the future if it's a problem, but for now I would say that we just run npm test during development and use npm run test:all before pushing (or just checking the result of the CI).
Janther
force-pushed
the
comparing-bytecode
branch
from
6ad2779 to
c169ab4
Compare
Janther
added
enhancement
Janther
force-pushed
the
comparing-bytecode
branch
3 times, most recently
from
5cc6b8d to
81f17d8
Compare
tests_config/run_spec.js
Outdated
| @@ -38,6 +38,35 @@ const unstableAstTests = new Map( | |||
| }) | |||
| ); | |||
|
|
|||
| const astChangedTests = new Map( | |||
There was a problem hiding this comment.
Two questions here:
- Is the name of this variable accurate?
- Why is the bytecode comparison only done for these files? Why don't we do it for all of them?
There was a problem hiding this comment.
Ohh, the bytecode comparison is only done for these files, I understand now. I still think the name of the variable is not very descriptive.
There was a problem hiding this comment.
- Just changed the name slightly to be more precise.
- For now I believe AST comparison should be enough and byte-code comparison applies only on the cases when we choose to change the AST.
I agree that best case would be to ultimately compare byte codes but I also see all of the work needed to do that properly.
Making sure every test is compilable and also keeping track of major syntax changes across different compilers.
|
I would love to run this bytecode comparison in all our test files. This is a lot of work, because:
But I think this PR is already too big, and it covers good ground. I think we can merge it and create an issue to do the full thing another day. |
Janther
force-pushed
the
comparing-bytecode
branch
from
dc25fba to
6aadd9f
Compare
There are 3 different cases where we make changes to the AST while parsing.
HexLiteral: if the plugin needs to change the quote of aHexLiteralbecause of thesingleQuotesetting, this is better done while parsing to avoid throwing an error in the tests for a different AST after formatting.uintandintare aliases foruint256andint256respectively. These will generate different ASTs when using a different setting forexplicitTypes- ie:
a / b * cwill be formatted as(a / b) * c.This clearly implies a change in the AST.
As nice as these changes to the AST can be for formatting, the compiled bytecode should remain the same. To enforce this, this PR will add the ability to compare the bytecode of some contracts designed to trigger all of these AST changes.