Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
eof: Pass
eofVersion
flag toEVMDialect
#15467eof: Pass
eofVersion
flag toEVMDialect
#15467Changes from all commits
8d8ac1c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make the EOF version a parameter in
hasStaticCall()
,hasCreate2()
andhasExtCodeHash()
. I wasn't sure about it back when I reviewed this PR, but now I saw this bit again in #15456 and I think it is necessary after all.I actually just tried to do this myself as a quick tweak, but looks like it requires the EOF version to be passed into a few more places. And in some of those places it actually needs to be handled since they make assumptions based on the availability of these instructions. For example there are fallback to
call
whenstaticcall
is not available, but on EOF they should result inextstaticcall
instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason we use hard-coded EVM version here is that historically this was always the one that had all possible builtins. With EOF this changes so I guess we'll have to use a different strategy for checking if the thing is a builtin - we should check both EOF and non-EOF dialect here and call
validateInstruction()
for whichever has the instruction (or maybe even for both).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if you want this to be added in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be best to still do it here. I wouldn't be against doing it later in another PR in principle, but we have so many of these TODOs now that I'm a bit afraid that things like this will just get forgotten and lost, only to surface as bugs much later :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. But we need test for this. Current
SyntaxTest
does not supportcompileToEOF
flag. I would add it toEVMVersionRestrictedTestCase
constructor to have it available in all derived TaseCase classes. WDYT? If so here on additional PR? :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has to be done in this PR because CI requires to cover all possible error codes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way sounds fine, but it's related so having it here is probably a good idea. And yeah, the error code - we have a whitelist for unused codes in
scripts/error_codes.py
so you could temporarily add it there if necessary, but that's always a last resort :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though one thing to consider here - we have two different approaches to these settings, and you need to choose one:
EVMVersion
setting: you set a supported range of versions in the test, but a single isoltest run always executes only for one specific version (selected via--evm-version
). If that matches the range, the test runs. Otherwise it's skipped. To get full coverage we run soltest with all relevant EVM versions in CI (viasoltest_all.sh
).compileViaYul
setting in semantic tests: you select compilation via Yul or not or both. And each run does exactly that, running it twice if necessary. There's no CLI option.The downside of (1) is that you'll have to run tests twice locally to test both EOF and legacy. It also adds another dimension to our test matrix. Currently we have two dimensions: optimizer on/off and EVM version. EOF version will be the third one, so you'll basically have to multiply the number of runs by two (though fortunately we can exclude older EVM versions from that, which lowers that number significantly).
Also, with (1) we end up unnecessarily running tests that are not affected by the flag multiple times, so it's only good for things that affect almost every part of the compiler.
The upside is that you get it supported almost for free in all test cases just by inheriting from
EVMVersionRestrictedTestCase
, while compiling things multiple times for (2) requires adding special support for it in each case. This matters especially for the smaller test cases (i.e. the ones where you used the flag fromCommonOptions
), where we definitely won't be going out of our way to make them use EOF, but it would be nice to have for more comprehensive coverage.Since we already have that
--eof-version
flag in soltest and the option is closely related to EVM version, I think it still makes the most sense to go with (1). And we'll need to add that additional run with EOF enabled anyway, because otherwise the tests using EOF versions fromCommonOptions
now would still only be tested with legacy.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, in short, my suggestion would be to extend
EVMVersionRestrictedTestCase
to make it support a setting likeEOFVersion
, which takes a list of comma-separated values: for now onlylegacy
and>=1
. The values should really be ranges, like for EVM, but since we only have one version, I'd not bother with that - I'd just make it support>=1
as a single, hard-coded value. If we ever get EOF v2, it will be easy to extend it by just changing how the value is parsed, without having to update it in all the tests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We agreed on addind EOF support for AsmAnalysis in a separated PR #15471