Skip to content
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

EVM instructions part 2 #34

Merged
merged 7 commits into from
Jun 16, 2018
Merged

EVM instructions part 2 #34

merged 7 commits into from
Jun 16, 2018

Conversation

chfast
Copy link
Member

@chfast chfast commented Jun 11, 2018

Changes:

  • evmc_instruction enum renamed to evmc_opcode and items prefixed with OP_.
  • names tables now also depend on EVM revision argument.

Follow up of #33.

@chfast chfast requested review from axic and gumb0 June 11, 2018 18:00
NULL,
NULL,
"INVALID",
"SELFDESTRUCT",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be SUICIDE in Frontier :)

And technically INVALID should only be added at a later hard fork if were are pedantic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The INVALID EIP does not have the date specified. I think we can assume that this is a kind of change that affects EVM also back in time.
The similar case is for SELFDESTRUCT (EIP-6) which was not the part of Homestead HF officially.

@@ -90,7 +90,7 @@ TEST(instructions, byzantium_hard_fork)

TEST(instructions, name_gas_cost_equivalence)
{
for (auto rev = EVMC_FRONTIER; rev <= EVMC_CONSTANTINOPLE;
for (auto rev = EVMC_FRONTIER; rev <= EVMC_LATEST_REVISON;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REVISON -> REVISION

EVMC_CONSTANTINOPLE = 5
EVMC_CONSTANTINOPLE = 5,

EVMC_LATEST_REVISON = EVMC_CONSTANTINOPLE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should be rather LATEST_STABLE_REVISION and point to BYZANTIUM?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, probably yes. Would you use it anywhere in Hera?

Maybe name it DEFAULT_REVISION?


#include <evmc/instructions.h>

static const char* constantinople_names[256] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lists are hard to maintain.
Maybe at least add delimiting comment lines like

   // 0x0
   "STOP",
   "ADD",
...
   // 0x10
...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added numbers in comments.

@chfast chfast force-pushed the evm-instructions branch 2 times, most recently from e9a7023 to 9b1ebea Compare June 16, 2018 10:12
@chfast chfast force-pushed the evm-instructions branch from 9b1ebea to 41b3501 Compare June 16, 2018 10:32
@chfast chfast merged commit 8489aa4 into master Jun 16, 2018
@chfast chfast deleted the evm-instructions branch June 16, 2018 10:35
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