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

Add int support #3

Merged
merged 2 commits into from
Jul 12, 2018
Merged

Add int support #3

merged 2 commits into from
Jul 12, 2018

Conversation

lucasnar
Copy link
Contributor

Resolves #1 (originally: blockscout/blockscout#363).

Description

The int type in Solidity differs from uint in which it includes negative numbers in its range of possible values. For instance, an 8-bit int can contain any number from -127 to 127, while an 8-bit uint can contain any number from 0 to 255. Also, if we try to define an 8-bit integer outside of those ranges, we will get an overflow (and possibly an error or unexpected behaviour from the machine that is running the code). Related resource on unsigned and signed integers: http://kias.dyndns.org/comath/13.html.

While this lib could handle uint types, it had no support for int types. This PR adds support for them, by enabling both encoding and decoding of int types in the ABI.TypeEncoder and ABI.TypeDecoder.

defp encode_int(data), do: <<data::signed-256>>

defp signed_overflow?(n, max_bits) do
n < (:math.pow(2, max_bits-1) * -1) + 1 || n > (:math.pow(2, max_bits-1)) - 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ayrat555, this will mathematically check if the given data causes an integer overflow with the given number of bits. I see the overflow checking is implemented differently in the uint encoding (by converting binaries in Elixir and counting bytes). Do you know why it was implemented that way and not with a formula (probably something like n > (:math.pow(2, max_bits)) - 1)?

I am asking because I am a little bit worried that I might not be seeing something and my solution is actually wrong.

Choose a reason for hiding this comment

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

Are you sure :math.pow is faster? Usually at the C level, powers of 2 are done with bit shifts. It may be that counting bytes in a binary is faster in Elixir/Erlang than :math:pow. I'm not sure if :math.pow is optimized for base 2 in the C code of the BIF.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if it's faster, but I couldn't use the same approach used for uint because it relies on :binary.encode_unsigned/1, which doesn't work for negative values. This is the simplest approach I could come up with. I didn't even think too much about the (possible) performance issue because I thought my solution could be even faster than the uint one because I was just making a simple calculation, while the other one was converting to binary, counting bytes and comparing with another number (that also had to be calculated). But that was just a feeling, so thanks for bringing that up. Do you think we can improve the performance?

You can see the uint encoding and overflow check here: https://github.com/Lucasnar/ex_abi/blob/92e14bf674e41672d6958e2e85834867aee14886/lib/abi/type_encoder.ex#L252

Choose a reason for hiding this comment

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

I would recommend running benchee tests to be sure, but that can be done in other work.

@ayrat555
Copy link
Collaborator

@lucasnar can you bump the library version?

@amandasposito
Copy link

@ayrat555 Do you think we can merge and bump the version in another PR? @lucasnar is out sick and we don't have the access rights to send a bump commit to this PR.

@ayrat555 ayrat555 merged commit c1584c0 into poanetwork:master Jul 12, 2018
@ayrat555
Copy link
Collaborator

@amandasposito https://hexdocs.pm/ex_abi/0.1.13 done

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.

5 participants