Add Istanbul to evmc_revision #174
Conversation
include/evmc/evmc.h
Outdated
| @@ -706,6 +706,7 @@ enum evmc_revision | |||
| EVMC_SPURIOUS_DRAGON = 3, | |||
| EVMC_BYZANTIUM = 4, | |||
| EVMC_CONSTANTINOPLE = 5, | |||
| EVMC_ISTANBUL = 6, | |||
|
|
|||
| EVMC_LATEST_REVISION = EVMC_CONSTANTINOPLE | |||
There was a problem hiding this comment.
I'm still not sold that should point to the unstable version.
There was a problem hiding this comment.
@chfast can you explain the reasoning behind?
There was a problem hiding this comment.
You can iterate over all revisions or make a table for all of them.
evmc/test/unittests/test_instructions.cpp
Line 116 in 0ff7c38
There was a problem hiding this comment.
Okay that is reasonable. Actually what convinced me is that it would be hard to keep evmc version in line with hardforks and ensure that clients are also always up to date, would we want to set latest_version always to the latest active version.
However, I think we should add a comment to it.
There was a problem hiding this comment.
Actually, what other enums usually do is use MAX_<name>. Perhaps the best naming would be EVMC_MAX_REVISION to fulfill this role. What do you think?
There was a problem hiding this comment.
Yes, this is mostly this case, just "latest" sounds better than "max". But it might be ok to change it to MAX for people that already know this idiom. Or just add proper documentation.
I agree that having something like "latest stable" is bad idea. EVM implementation should be responsible for bumping the default revision on their own if they have any.
There was a problem hiding this comment.
Yes, this is mostly this case, just "latest" sounds better than "max".
I think latest very much suggests it points to the latest stable. Hence my confusion all along.
|
@chfast ok to merge this? |
Closes #134. Ref ethereum/EIPs#1679.