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

Support CREATE2 opcode in Constantinople #45

Merged
merged 3 commits into from
Jul 25, 2018
Merged

Support CREATE2 opcode in Constantinople #45

merged 3 commits into from
Jul 25, 2018

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Jul 24, 2018

No description provided.

@gumb0
Copy link
Member Author

gumb0 commented Jul 24, 2018

Still need to add it to instructions lib.

chfast
chfast previously requested changes Jul 24, 2018
@@ -70,7 +70,8 @@ enum evmc_call_kind
EVMC_CALL = 0, /**< Request CALL. */
EVMC_DELEGATECALL = 1, /**< Request DELEGATECALL. The value param ignored. */
EVMC_CALLCODE = 2, /**< Request CALLCODE. */
EVMC_CREATE = 3 /**< Request CREATE. Semantic of some params changes. */
EVMC_CREATE = 3, /**< Request CREATE. Semantic of some params changes. */
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the "Semantic of some params changes" part in both cases.

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 these should state which evmc_revision they are valid in (frontier, homestead, byzantium, constantinople).

@gumb0 gumb0 removed the in progress label Jul 25, 2018
*
* Ignored unless kind == EVMC_CREATE2.
*/
struct evmc_uint256be salt;
Copy link
Member

Choose a reason for hiding this comment

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

How about we name it create_salt?

Copy link
Member

Choose a reason for hiding this comment

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

create2_salt for explicitlness?

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise creation_salt?

Copy link
Member

Choose a reason for hiding this comment

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

create2_salt 👍
Similarly to create_address.

/**
* The optional value used in new contract address construction.
*
* Ignored unless kind == EVMC_CREATE2.
Copy link
Member

@axic axic Jul 25, 2018

Choose a reason for hiding this comment

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

unless kind is EVMC_CREATE2.

@@ -12,7 +12,6 @@ static_assert(sizeof(evmc_uint256be) == 32, "evmc_uint256be is too big");
static_assert(sizeof(evmc_address) == 20, "evmc_address is too big");
static_assert(sizeof(evmc_result) <= 64, "evmc_result does not fit cache line");
static_assert(sizeof(evmc_instance) <= 64, "evmc_instance does not fit cache line");
static_assert(sizeof(evmc_message) <= 18 * 8, "evmc_message not optimally packed");
Copy link
Member

Choose a reason for hiding this comment

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

Should this be kept with an updated size?

Copy link
Member

Choose a reason for hiding this comment

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

It does not anything useful. I was just checking obvious struct fields alignment.

@gumb0
Copy link
Member Author

gumb0 commented Jul 25, 2018

Renamed to create2_salt

@axic
Copy link
Member

axic commented Jul 25, 2018

This also needs an ABI version bump.

@gumb0
Copy link
Member Author

gumb0 commented Jul 25, 2018

Bumped ABI version to 2

@axic axic dismissed chfast’s stale review July 25, 2018 16:13

It was updated.

@chfast chfast merged commit 2bece50 into master Jul 25, 2018
@chfast chfast deleted the create2 branch July 25, 2018 16:21
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