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

EEI: Change address type from integer to bytes #105

Merged
merged 1 commit into from
Oct 7, 2018

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Jul 3, 2018

No description provided.

@chfast chfast requested a review from axic July 3, 2018 12:22
@chfast chfast mentioned this pull request Jul 5, 2018
@axic axic added this to Proposed in Revision 4 Jul 30, 2018
@@ -6,7 +6,7 @@ The Ethereum Environment Interface exposes the core Ethereum API to the eWASM en

We define the following Ethereum data types:
- `bytes`: an array of bytes with unrestricted length
- `address`: a 160 bit number, represented as a 20 bytes long little endian unsigned integer in memory
- `address`: an array of 20 bytes
- `u128`: a 128 bit number, represented as a 16 bytes long little endian unsigned integer in memory
- `u256`: a 256 bit number, represented as a 32 bytes long little endian unsigned integer in memory
Copy link
Member

Choose a reason for hiding this comment

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

Should we actually consider u128/u256 numbers or just an array of bytes?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. I think that would not be sufficient. You would have to explain where these bytes come from, i.e. how numbers where serialized to bytes, i.e. endianness.

In case of address this is not needed because an address is a well specified part of a hash, where a hash is defined as bytes, not a number.

Copy link
Member

Choose a reason for hiding this comment

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

u128 should be defined as an integer value, since it (usually?) denotes an ether value and native arithmetic and comparison operators should work on them in the WASM environment (by experimental u128 or otherwise).
u256 is very difficult because a hash value should not be byte-swapped but the difficulty should be, as it is a quantity.
Byte ordering is therefore highly dependent on context in the case of u256.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So make two types.

@@ -6,7 +6,7 @@ The Ethereum Environment Interface exposes the core Ethereum API to the eWASM en

We define the following Ethereum data types:
- `bytes`: an array of bytes with unrestricted length
- `address`: a 160 bit number, represented as a 20 bytes long little endian unsigned integer in memory
- `address`: an array of 20 bytes
Copy link
Member

Choose a reason for hiding this comment

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

Practically this means it is "big endian"? In evmc it still says big endian.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe it means big endian, but I usually have trouble reasoning about 160-bit numbers.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is currently a case where a 160-bit type should be treated as a value.
The Solidity documentation states that only comparison operators are defined for the "address" type. This puts the type in a difficult situation where it is mostly not a quantity, but can be compared like an integer using operators like <, >, <=, and >=.
I think that the type should not have any notion of byte ordering other than defined by the hash function used to generate it. A correct address value should be the last 20 bytes of the hash of the public key, as usual.

Perhaps this reflects on a needed change in EVMC?

Copy link
Member

Choose a reason for hiding this comment

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

Solidity actually considers them numbers (as they derive off IntegerType).

EVM considers them numbers too, since they are "right aligned" in the stack slot.

@chfast chfast moved this from Proposed to Under discussion in Revision 4 Aug 16, 2018
@axic axic merged commit ec7e053 into ewasm:master Oct 7, 2018
@axic axic moved this from Under discussion to Decided in Revision 4 Oct 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Revision 4
Decided
Development

Successfully merging this pull request may close these issues.

None yet

3 participants