-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Deprecate support for byzantium
and older EVM versions
#14591
Conversation
if (version <= EVMVersion::constantinople()) | ||
ret.errors.append(formatError( | ||
Error::Type::Warning, | ||
"general", | ||
"Support for constantinople and older EVM versions is deprecated and will be removed in the future." | ||
)); |
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.
I decided to put it in the interface (rather than compiler/yul stack) to avoid having to deal with filtering out the warning in soltest. The EVM version is a CLI argument so the warning would be present or missing in every test based on that.
The consequence is that it has no error code and that it won't appear when the use is not through CLI or Standard JSON (which I guess never happens for a normal user). The upside is that I don't have to track what uses the EVM version and instead I issue the warning at the point where the option gets its value.
a59cff0
to
466f7bc
Compare
b6b2262
to
2138478
Compare
2138478
to
5ef5c0f
Compare
b372dc9
to
1bfcbd3
Compare
constantinople
and older EVM versionsbyzantium
and older EVM versions
16cbce4
to
23b7505
Compare
1bfcbd3
to
4dc4a56
Compare
Can you bump the evm version in https://docs.soliditylang.org/en/latest/using-the-compiler.html#input-description :-)? |
4dc4a56
to
0e5790b
Compare
Done. |
0e5790b
to
afd0173
Compare
afd0173
to
a333896
Compare
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.
LGTM
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.
Implementation looks good, but the wording of "support deprecated" is odd; deprecation is discouragement of use, so if you deprecate something, you're no longer providing support, and are advising against its use.
@@ -147,14 +147,14 @@ Target Options | |||
Below is a list of target EVM versions and the compiler-relevant changes introduced | |||
at each version. Backward compatibility is not guaranteed between each version. | |||
|
|||
- ``homestead`` | |||
- ``homestead`` (*support deprecated*) |
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.
Support can't be deprecated, so I'd just change this to
- ``homestead`` (*support deprecated*) | |
- ``homestead`` (*deprecated*) |
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.
I intentionally worded it that way. It's not the EVM version that we're deprecating - we have no control over that and I have no idea if these old EVMs are considered deprecated or not. AFAIK there's no deprecation procedure for hard forks. They just naturally fall out of use. This fuzziness about how relevant they still are is exactly what has been holding us back from doing this deprecation earlier.
What we're doing is deprecating compiler's support for these EVM versions.
I agree that the wording is a bit awkward but I did not find a better way to word it concisely.
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.
Eh, alright, fair enough. Merge away.
@@ -7,6 +7,7 @@ Language Features: | |||
Compiler Features: | |||
* Commandline Interface: Add ``--no-import-callback`` option that prevents the compiler from loading source files not given explicitly on the CLI or in Standard JSON input. | |||
* Commandline Interface: Use proper severity and coloring also for error messages produced outside of the compilation pipeline. | |||
* EVM: Deprecate support for "homestead", "tangerineWhistle", "spuriousDragon" and "byzantium" EVM versions. |
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.
* EVM: Deprecate support for "homestead", "tangerineWhistle", "spuriousDragon" and "byzantium" EVM versions. | |
* EVM: Deprecate "homestead", "tangerineWhistle", "spuriousDragon" and "byzantium" EVM versions. |
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.
Slapping a change request since it's already approved, and I want to discuss the wording (as explained in the above comments) before it gets merged.
@@ -147,14 +147,14 @@ Target Options | |||
Below is a list of target EVM versions and the compiler-relevant changes introduced | |||
at each version. Backward compatibility is not guaranteed between each version. | |||
|
|||
- ``homestead`` | |||
- ``homestead`` (*support deprecated*) |
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.
Eh, alright, fair enough. Merge away.
a333896
to
28a4567
Compare
Support discontinued? |
But it's not discontinued yet. Just deprecated :) |
…as well - Until now it was being removed along with the leading/trailing whitespace that we strip so it did not matter. Now the EVM deprecation warning can get before this warning, preserving the whitespace and making CLI tests fail.
28a4567
to
127a390
Compare
Resolves #14475.
Depends on #14590.Merged.